gitforge/modules/process/graceful_cancel.go
Mathieu Fenniak d4e2bf7396 fix: intermittent test failure in process cancellation (#13293)
Test failure: https://codeberg.org/forgejo/forgejo/actions/runs/174007/jobs/2/attempt/1   Have only observed once and can't reproduce locally, but likely caused by #13263 / #13289.

```
--- FAIL: TestGetDiffPreview (0.20s)
    --- FAIL: TestGetDiffPreview/empty_branch,_same_results (0.02s)
        diff_test.go:132:
            	Error Trace:	/workspace/forgejo/forgejo/services/repository/files/diff_test.go:132
            	Error:      	Received unexpected error:
            	            	unable to run diff-index pipeline in temporary repo: exec: canceling Cmd: invalid argument
            	Test:       	TestGetDiffPreview/empty_branch,_same_results
FAIL
```

pidfd_open is [documented](https://www.man7.org/linux/man-pages/man2/pidfd_open.2.html) to return the EINVAL (invalid argument) error if the pid is not valid, which could happen if the process terminates itself before Forgejo sends `SIGTERM`.  This PR adds a check for that error and stops `Cancel`, with a minor refactor to avoid code duplication.

### Tests for Go changes

- I added test coverage for Go changes...
  - [x] in their respective `*_test.go` for unit tests.
  - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
  - [x] `make pr-go` before pushing

### Documentation

- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13293
Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org>
2026-07-04 04:49:36 +02:00

82 lines
2.7 KiB
Go

// Copyright 2026 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: GPLv3-or-later
package process
import (
"os"
"os/exec"
"slices"
"syscall"
"time"
)
// Identify errors that can occur in kill or wait that indicate the kill was actually successful as the process doesn't
// exist anymore.
func killErrorSafeToIgnore(err error) bool {
return isErrno(err,
syscall.ESRCH, // No such process
syscall.ECHILD, // No child processes
)
}
func isErrno(err error, errnoList ...syscall.Errno) bool {
errno, is := err.(syscall.Errno)
if is {
if slices.Contains(errnoList, errno) {
return true
}
}
syscallErr, isSyscallErr := err.(*os.SyscallError) // errno may be wrapped in SyscallError
if isSyscallErr && isErrno(syscallErr.Err, errnoList...) {
return true
}
return false
}
// Create an implementation of `exec.Cmd.Cancel` which gracefully shuts down the process, sending SIGTERM, waiting a
// grace period, and sending SIGKILL.
func genericGracefulCancel(cmd *exec.Cmd) func() error {
return func() error {
// Send SIGTERM to the entire process group by sending to the negative PID.
err := syscall.Kill(-cmd.Process.Pid, syscall.SIGTERM)
if killErrorSafeToIgnore(err) {
return nil
} else if err != nil {
return err
}
// We can't just `cmd.Process.Wait()` because another goroutine will be doing that concurrently, but
// conceptually that is what we want to do. This approach sends Signal(0) to the process until that returns an
// error (indicating that the process is terminated) or until the grace interval is complete, with a delay
// between loops.
//
// The arbitrary delay between loops isn't ideal when compared to letting the OS wake us when the process is
// done (as the Linux pidfd implementation does), but it's cross-platform. As this is already an edge-case
// where a subprocess has an abnormal termination caused by Forgejo, which isn't very frequent, a loop and wait
// implementation shouldn't have significant performance impact.
deadline := time.Now().Add(TerminateGraceTimeout)
for time.Until(deadline) >= 0 {
err := cmd.Process.Signal(syscall.Signal(0))
if err == os.ErrProcessDone {
// Process has terminated.
return nil
} else if err != nil {
return err
}
// Don't busy-loop -- take a small breather to avoid 100% CPU usage.
time.Sleep(50 * time.Millisecond)
}
// After sending SIGTERM, we hit the terminate grace timeout and the subprocess is still running. Send SIGKILL
// to the process group as a fallback.
if NotifyTerminateGraceExhausted != nil {
NotifyTerminateGraceExhausted(cmd)
}
err = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
if killErrorSafeToIgnore(err) {
return nil
}
return err
}
}