fix: stop host-mode jobs from leaking processes on Windows (#1080)

## Problem On host-mode Windows runners, a job can leave processes running after it finishes, and those leftovers hold file handles that block deletion of the workspace. Today a step's tree is only torn down when the step is *cancelled* (`process.Killer`). A step that completes leaves whatever it spawned alive, and the existing workspace scan in `terminateRunningProcesses` misses two shapes of leftover: orphans whose parent already exited (no tree to walk, and their executable often lives outside the workspace), and processes that merely *run in* the workspace but reference no path from it — `Win32_Process` exposes no working directory, so the scan cannot match them. ## Solution Two additions in `internal/pkg/process`, both no-ops outside Windows: - **`process.Group`** — a job-scoped Windows Job Object created with `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`. Step processes are assigned to it before they get their own `Killer`, so the step's job nests inside the job's: cancellation still kills exactly the step's tree, while `Remove` closing the group makes the kernel terminate everything still assigned, whatever its parentage. The kernel also drops the handle when the runner exits, so a crashed runner cannot strand processes. - **`process.KillProcessesWithCWDUnder`** — a best-effort net for processes that never joined the job (started via a service or scheduled task). It reads each process's working directory from its PEB and terminates those under a workspace dir. Processes it cannot open are skipped. --------- Co-authored-by: silverwind <me@silverwind.io> Reviewed-on: https://gitea.com/gitea/runner/pulls/1080 Reviewed-by: silverwind <2021+silverwind@noreply.gitea.com>
2026-07-22 20:47:45 +00:00 · 2026-07-22 15:04:07 +00:00
parent 8af385d147
commit 7bec310002
9 changed files with 645 additions and 43 deletions
--- a/internal/pkg/process/group_others.go
+++ b/internal/pkg/process/group_others.go
@@ -0,0 +1,28 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+//go:build !windows
+
+package process
+
+import "os"
+
+// Group is a no-op outside Windows: process groups already reclaim a step's
+// tree, and an orphan does not block workspace deletion the way an open Windows
+// handle does.
+type Group struct{}
+
+// NewGroup returns a Group that owns nothing.
+func NewGroup() (*Group, error) {
+	return &Group{}, nil
+}
+
+// Assign does nothing.
+func (g *Group) Assign(_ *os.Process) error {
+	return nil
+}
+
+// Close does nothing.
+func (g *Group) Close() error {
+	return nil
+}
--- a/internal/pkg/process/group_windows.go
+++ b/internal/pkg/process/group_windows.go
@@ -0,0 +1,86 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package process
+
+import (
+	"os"
+	"sync"
+	"unsafe"
+
+	"golang.org/x/sys/windows"
+)
+
+// Group is a job-scoped Windows Job Object holding every process the job's steps
+// start; closing it terminates whatever is still assigned, reaching orphans the
+// per-step Killer misses (a completed step's leftover has no parent to walk from).
+type Group struct {
+	mu  sync.Mutex
+	job windows.Handle
+}
+
+// NewGroup creates the job object. Closing it is what terminates the leftovers,
+// so the caller must Close it when the job ends.
+func NewGroup() (*Group, error) {
+	job, err := windows.CreateJobObject(nil, nil)
+	if err != nil {
+		return nil, err
+	}
+
+	info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{
+		BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{
+			LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE,
+		},
+	}
+	if _, err := windows.SetInformationJobObject(
+		job,
+		windows.JobObjectExtendedLimitInformation,
+		uintptr(unsafe.Pointer(&info)),
+		uint32(unsafe.Sizeof(info)),
+	); err != nil {
+		_ = windows.CloseHandle(job)
+		return nil, err
+	}
+
+	return &Group{job: job}, nil
+}
+
+// Assign adds a started process to the job; anything it spawns afterwards joins
+// too. Call before NewKiller so the step's job nests inside this one. Nil-safe.
+func (g *Group) Assign(p *os.Process) error {
+	if g == nil || p == nil {
+		return nil
+	}
+
+	g.mu.Lock()
+	defer g.mu.Unlock()
+	if g.job == 0 {
+		return nil
+	}
+
+	h, err := windows.OpenProcess(windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, false, uint32(p.Pid))
+	if err != nil {
+		return err
+	}
+	defer func() { _ = windows.CloseHandle(h) }()
+
+	return windows.AssignProcessToJobObject(g.job, h)
+}
+
+// Close drops the job handle, terminating every process still assigned to it.
+// Nil-safe and idempotent.
+func (g *Group) Close() error {
+	if g == nil {
+		return nil
+	}
+
+	g.mu.Lock()
+	defer g.mu.Unlock()
+	if g.job == 0 {
+		return nil
+	}
+
+	h := g.job
+	g.job = 0
+	return windows.CloseHandle(h)
+}
--- a/internal/pkg/process/group_windows_test.go
+++ b/internal/pkg/process/group_windows_test.go
@@ -0,0 +1,113 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package process
+
+import (
+	"fmt"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+// startOrphanMaker starts a process that spawns a detached, long-lived child and
+// exits, leaving the child parentless. Returns the cmd and the child's PID file.
+func startOrphanMaker(t *testing.T) (*exec.Cmd, string) {
+	t.Helper()
+
+	pidFile := filepath.Join(t.TempDir(), "child.pid")
+	script := fmt.Sprintf(
+		`$c = Start-Process powershell -PassThru -ArgumentList '-NoProfile','-Command','Start-Sleep -Seconds 600'; `+
+			`Set-Content -LiteralPath %q -Value $c.Id`, pidFile)
+
+	cmd := exec.Command("powershell.exe", "-NoProfile", "-Command", script)
+	require.NoError(t, cmd.Start())
+	t.Cleanup(func() { _ = cmd.Process.Kill() })
+
+	return cmd, pidFile
+}
+
+// awaitChildPID waits until the spawned child has reported its PID and is running.
+func awaitChildPID(t *testing.T, pidFile string) int {
+	t.Helper()
+
+	var childPID int
+	require.Eventually(t, func() bool {
+		b, err := os.ReadFile(pidFile)
+		if err != nil {
+			return false
+		}
+		s := strings.TrimSpace(string(b))
+		if s == "" {
+			return false
+		}
+		childPID, _ = strconv.Atoi(s)
+		return childPID > 0 && processAlive(childPID)
+	}, 20*time.Second, 200*time.Millisecond, "child process should start")
+
+	return childPID
+}
+
+// TestGroupClosePropagatesToOrphan covers what the per-step Killer cannot: a
+// completed step's orphan, reachable only by closing the job object.
+func TestGroupClosePropagatesToOrphan(t *testing.T) {
+	group, err := NewGroup()
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = group.Close() })
+
+	cmd, pidFile := startOrphanMaker(t)
+	require.NoError(t, group.Assign(cmd.Process))
+
+	childPID := awaitChildPID(t, pidFile)
+
+	require.NoError(t, cmd.Wait()) // the step process exits cleanly, like a passing step
+	require.Eventually(t, func() bool {
+		return !processAlive(cmd.Process.Pid)
+	}, 20*time.Second, 200*time.Millisecond, "the step process should have exited on its own")
+
+	require.True(t, processAlive(childPID), "orphan should outlive its parent")
+
+	require.NoError(t, group.Close())
+	require.Eventually(t, func() bool {
+		return !processAlive(childPID)
+	}, 20*time.Second, 200*time.Millisecond, "closing the job must terminate the orphan")
+}
+
+// TestGroupNestsWithKiller: assigned to the group first and the step's Killer
+// second, cancelling the step still kills exactly that step's tree.
+func TestGroupNestsWithKiller(t *testing.T) {
+	group, err := NewGroup()
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = group.Close() })
+
+	cmd := exec.Command("powershell.exe", "-NoProfile", "-Command", "Start-Sleep -Seconds 600")
+	require.NoError(t, cmd.Start())
+	t.Cleanup(func() { _ = cmd.Process.Kill() })
+
+	require.NoError(t, group.Assign(cmd.Process))
+
+	// Nesting must be accepted, else cancellation falls back to a single-process kill.
+	killer, err := NewKiller(cmd.Process)
+	require.NoError(t, err, "the step's job object must nest inside the group's")
+	defer killer.Close()
+
+	require.NoError(t, killer.Kill())
+	require.Eventually(t, func() bool {
+		return !processAlive(cmd.Process.Pid)
+	}, 20*time.Second, 200*time.Millisecond, "cancelling the step should still kill its tree")
+}
+
+// TestGroupNilSafe covers the fallback path: when the job object cannot be
+// created the caller holds a nil *Group and must still be able to use it.
+func TestGroupNilSafe(t *testing.T) {
+	var group *Group
+	require.NoError(t, group.Assign(nil))
+	require.NoError(t, group.Close())
+	require.NoError(t, group.Close())
+}
--- a/internal/pkg/process/leftover.go
+++ b/internal/pkg/process/leftover.go
@@ -0,0 +1,29 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package process
+
+import (
+	"path/filepath"
+	"strings"
+)
+
+// pathWithin is the OS-agnostic core of dirContainsPath: dir and target must
+// already be cleaned, and fold folds case.
+func pathWithin(dir, target string, fold bool) bool {
+	if dir == "" || target == "" || dir == "." || target == "." {
+		return false
+	}
+	if fold {
+		dir = strings.ToLower(dir)
+		target = strings.ToLower(target)
+	}
+	if dir == target {
+		return true
+	}
+	sep := string(filepath.Separator)
+	if !strings.HasSuffix(dir, sep) {
+		dir += sep
+	}
+	return strings.HasPrefix(target, dir)
+}
--- a/internal/pkg/process/leftover_others.go
+++ b/internal/pkg/process/leftover_others.go
@@ -0,0 +1,14 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+//go:build !windows
+
+package process
+
+import "context"
+
+// KillProcessesWithCWDUnder is a no-op outside Windows: an open handle does not
+// block os.RemoveAll there.
+func KillProcessesWithCWDUnder(_ context.Context, _ []string) (int, error) {
+	return 0, nil
+}
--- a/internal/pkg/process/leftover_test.go
+++ b/internal/pkg/process/leftover_test.go
@@ -0,0 +1,41 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package process
+
+import (
+	"path/filepath"
+	"testing"
+)
+
+func TestPathWithin(t *testing.T) {
+	base := filepath.Join("base", "job1")
+
+	cases := []struct {
+		name   string
+		dir    string
+		target string
+		fold   bool
+		want   bool
+	}{
+		{"same dir", base, base, false, true},
+		{"direct child", base, filepath.Join(base, "app.exe"), false, true},
+		{"deep child", base, filepath.Join(base, "sub", "sub", "app.exe"), false, true},
+		{"parent is not within child", filepath.Join(base, "sub"), base, false, false},
+		{"name-prefix sibling spared", base, filepath.Join("base", "job10", "app.exe"), false, false},
+		{"unrelated", base, filepath.Join("other", "app.exe"), false, false},
+		{"empty dir", "", base, false, false},
+		{"empty target", base, "", false, false},
+		{"dot dir", ".", base, false, false},
+		{"case-sensitive miss", base, filepath.Join("base", "JOB1", "app.exe"), false, false},
+		{"case-insensitive hit", base, filepath.Join("base", "JOB1", "app.exe"), true, true},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := pathWithin(filepath.Clean(tc.dir), filepath.Clean(tc.target), tc.fold)
+			if got != tc.want {
+				t.Fatalf("pathWithin(%q, %q, fold=%v) = %v, want %v", tc.dir, tc.target, tc.fold, got, tc.want)
+			}
+		})
+	}
+}
--- a/internal/pkg/process/leftover_windows.go
+++ b/internal/pkg/process/leftover_windows.go
@@ -0,0 +1,182 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package process
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"os"
+	"path/filepath"
+	"unsafe"
+
+	"golang.org/x/sys/windows"
+)
+
+// dirContainsPath reports whether target is dir or lives beneath it,
+// case-insensitively, so a name-prefix sibling (job1 vs job10) is not a match.
+func dirContainsPath(dir, target string) bool {
+	return pathWithin(filepath.Clean(dir), filepath.Clean(target), true)
+}
+
+// KillProcessesWithCWDUnder terminates every process in the runner's session
+// whose cwd is one of dirs or below, catching leftovers the path scan misses
+// (Win32_Process exposes no cwd) that still pin a handle. Best-effort.
+func KillProcessesWithCWDUnder(ctx context.Context, dirs []string) (int, error) {
+	if len(dirs) == 0 {
+		return 0, nil
+	}
+
+	pids, err := listProcessIDs()
+	if err != nil {
+		return 0, err
+	}
+
+	self := uint32(os.Getpid())
+	var ownSession uint32
+	if err := windows.ProcessIdToSessionId(self, &ownSession); err != nil {
+		return 0, fmt.Errorf("look up own session: %w", err)
+	}
+
+	var killed int
+	var errs []error
+	for _, pid := range pids {
+		if ctx.Err() != nil {
+			break
+		}
+		// Never touch ourselves, System Idle (0) or System (4).
+		if pid == self || pid == 0 || pid == 4 {
+			continue
+		}
+		var session uint32
+		if err := windows.ProcessIdToSessionId(pid, &session); err != nil || session != ownSession {
+			continue
+		}
+		ok, err := terminateIfCWDUnder(pid, dirs)
+		if err != nil {
+			errs = append(errs, err)
+			continue
+		}
+		if ok {
+			killed++
+		}
+	}
+
+	return killed, errors.Join(errs...)
+}
+
+// terminateIfCWDUnder opens pid once and both reads its cwd and terminates it
+// through that one handle, so a PID recycled between the two cannot be hit.
+func terminateIfCWDUnder(pid uint32, dirs []string) (bool, error) {
+	h, err := windows.OpenProcess(windows.PROCESS_QUERY_INFORMATION|windows.PROCESS_VM_READ|windows.PROCESS_TERMINATE, false, pid)
+	if err != nil {
+		return false, nil // best-effort: gone, higher integrity, or another user
+	}
+	defer func() { _ = windows.CloseHandle(h) }()
+
+	cwd, err := processCWDHandle(h)
+	if err != nil || cwd == "" {
+		return false, nil
+	}
+
+	for _, d := range dirs {
+		if dirContainsPath(d, cwd) {
+			if err := windows.TerminateProcess(h, 1); err != nil {
+				return false, fmt.Errorf("terminate pid %d: %w", pid, err)
+			}
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
+// listProcessIDs returns the PIDs of every process in a Toolhelp snapshot.
+func listProcessIDs() ([]uint32, error) {
+	snap, err := windows.CreateToolhelp32Snapshot(windows.TH32CS_SNAPPROCESS, 0)
+	if err != nil {
+		return nil, err
+	}
+	defer func() { _ = windows.CloseHandle(snap) }()
+
+	var entry windows.ProcessEntry32
+	entry.Size = uint32(unsafe.Sizeof(entry))
+
+	pids := make([]uint32, 0, 256)
+	err = windows.Process32First(snap, &entry)
+	for err == nil {
+		pids = append(pids, entry.ProcessID)
+		err = windows.Process32Next(snap, &entry)
+	}
+	if !errors.Is(err, windows.ERROR_NO_MORE_FILES) {
+		return pids, err
+	}
+	return pids, nil
+}
+
+// processCWDHandle reads a process's working directory from its PEB (walking
+// PEB -> ProcessParameters -> CurrentDirectory.DosPath). h must carry
+// PROCESS_QUERY_INFORMATION|PROCESS_VM_READ.
+func processCWDHandle(h windows.Handle) (string, error) {
+	var pbi windows.PROCESS_BASIC_INFORMATION
+	var retLen uint32
+	if err := windows.NtQueryInformationProcess(h, windows.ProcessBasicInformation, unsafe.Pointer(&pbi), uint32(unsafe.Sizeof(pbi)), &retLen); err != nil {
+		return "", err
+	}
+	if pbi.PebBaseAddress == nil {
+		return "", errors.New("nil PEB base address")
+	}
+	pebAddr := uintptr(unsafe.Pointer(pbi.PebBaseAddress))
+
+	paramsAddr, err := readRemotePtr(h, pebAddr+unsafe.Offsetof(windows.PEB{}.ProcessParameters))
+	if err != nil || paramsAddr == 0 {
+		return "", err
+	}
+
+	// DosPath is the leading NTUnicodeString of CURDIR, so it shares CurrentDirectory's address.
+	dosPathAddr := paramsAddr + unsafe.Offsetof(windows.RTL_USER_PROCESS_PARAMETERS{}.CurrentDirectory)
+	length, err := readRemoteU16(h, dosPathAddr+unsafe.Offsetof(windows.NTUnicodeString{}.Length))
+	if err != nil || length == 0 {
+		return "", err
+	}
+	bufAddr, err := readRemotePtr(h, dosPathAddr+unsafe.Offsetof(windows.NTUnicodeString{}.Buffer))
+	if err != nil || bufAddr == 0 {
+		return "", err
+	}
+
+	u16 := make([]uint16, (length+1)/2) // round up so an odd Length cannot overflow the buffer
+	var n uintptr
+	if err := windows.ReadProcessMemory(h, bufAddr, (*byte)(unsafe.Pointer(&u16[0])), uintptr(length), &n); err != nil {
+		return "", err
+	}
+	if n != uintptr(length) {
+		return "", errors.New("short read of working directory")
+	}
+	return windows.UTF16ToString(u16), nil
+}
+
+// readRemotePtr reads a single pointer-sized value from another process.
+func readRemotePtr(h windows.Handle, addr uintptr) (uintptr, error) {
+	var v uintptr
+	var n uintptr
+	if err := windows.ReadProcessMemory(h, addr, (*byte)(unsafe.Pointer(&v)), unsafe.Sizeof(v), &n); err != nil {
+		return 0, err
+	}
+	if n != unsafe.Sizeof(v) {
+		return 0, errors.New("short pointer read")
+	}
+	return v, nil
+}
+
+// readRemoteU16 reads a single uint16 from another process.
+func readRemoteU16(h windows.Handle, addr uintptr) (uint16, error) {
+	var v uint16
+	var n uintptr
+	if err := windows.ReadProcessMemory(h, addr, (*byte)(unsafe.Pointer(&v)), unsafe.Sizeof(v), &n); err != nil {
+		return 0, err
+	}
+	if n != unsafe.Sizeof(v) {
+		return 0, errors.New("short uint16 read")
+	}
+	return v, nil
+}
--- a/internal/pkg/process/leftover_windows_test.go
+++ b/internal/pkg/process/leftover_windows_test.go
@@ -0,0 +1,91 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package process
+
+import (
+	"context"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"syscall"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/windows"
+)
+
+// startSleeperIn launches a long-running child whose cwd is dir but whose
+// executable lives outside it — what the path scan cannot see.
+func startSleeperIn(t *testing.T, dir string) *exec.Cmd {
+	t.Helper()
+	ping := filepath.Join(os.Getenv("SystemRoot"), "System32", "ping.exe")
+	cmd := exec.Command(ping, "-n", "60", "127.0.0.1")
+	cmd.Dir = dir
+	cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true}
+	require.NoError(t, cmd.Start())
+	t.Cleanup(func() {
+		_ = cmd.Process.Kill()
+		_, _ = cmd.Process.Wait()
+	})
+	return cmd
+}
+
+// processCWD opens pid and reads its working directory. Test-only; the reaper
+// shares one handle across read and kill via processCWDHandle.
+func processCWD(pid uint32) (string, error) {
+	h, err := windows.OpenProcess(windows.PROCESS_QUERY_INFORMATION|windows.PROCESS_VM_READ, false, pid)
+	if err != nil {
+		return "", err
+	}
+	defer func() { _ = windows.CloseHandle(h) }()
+	return processCWDHandle(h)
+}
+
+// awaitCWD waits until pid's PEB reports a readable cwd (not populated the
+// instant the process starts) and returns it.
+func awaitCWD(t *testing.T, pid int) string {
+	t.Helper()
+	var cwd string
+	require.Eventually(t, func() bool {
+		var err error
+		cwd, err = processCWD(uint32(pid))
+		return err == nil && cwd != ""
+	}, 5*time.Second, 50*time.Millisecond, "process cwd should become readable")
+	return cwd
+}
+
+func TestProcessCWDReadsWorkingDirectory(t *testing.T) {
+	dir := t.TempDir()
+	cmd := startSleeperIn(t, dir)
+	cwd := awaitCWD(t, cmd.Process.Pid)
+	require.True(t, dirContainsPath(dir, cwd), "cwd %q should be within %q", cwd, dir)
+}
+
+func TestKillProcessesWithCWDUnderTerminatesMatch(t *testing.T) {
+	dir := t.TempDir()
+	// Executable outside dir and arguments carry no workspace path, so only the
+	// working directory links this process to dir.
+	cmd := startSleeperIn(t, dir)
+	awaitCWD(t, cmd.Process.Pid)
+
+	killed, err := KillProcessesWithCWDUnder(context.Background(), []string{dir})
+	require.NoError(t, err)
+	require.GreaterOrEqual(t, killed, 1)
+
+	require.Eventually(t, func() bool {
+		return !processAlive(cmd.Process.Pid)
+	}, 5*time.Second, 50*time.Millisecond, "matched process should exit")
+}
+
+func TestKillProcessesWithCWDUnderSparesUnrelated(t *testing.T) {
+	workspace := t.TempDir()
+	elsewhere := t.TempDir()
+	cmd := startSleeperIn(t, elsewhere)
+	awaitCWD(t, cmd.Process.Pid)
+
+	_, err := KillProcessesWithCWDUnder(context.Background(), []string{workspace})
+	require.NoError(t, err)
+	require.True(t, processAlive(cmd.Process.Pid), "process outside owned dirs must be spared")
+}