3 Commits

Author SHA1 Message Date
Nicolas
c749e52bb7 fix(cleanup): kill Windows step process tree on cancel to avoid hang (#1011)
## Problem

Cancelling a job on a Windows host runner can leave the spawned process
tree running and hang the runner. When a step launches a shell that
starts a child which in turn spawns further GUI/background processes,
cancelling the job kills only the direct child (the default
`exec.CommandContext` behaviour). The surviving descendants inherited
the step's stdout/stderr pipe, so the read end never hit EOF and
`cmd.Wait()` blocked forever.

Because the step executor never returned:
- the orphaned processes kept running (the cancelled work was not
  actually stopped), and
- end-of-job cleanup (`Remove` → `terminateRunningProcesses`) was never
  reached, so the runner appeared to go offline / stop picking up jobs.

`CREATE_NEW_PROCESS_GROUP` does not help here — it affects Ctrl-C signal
delivery, not handle inheritance or tree termination.

## Fix

- Assign each Windows step process to a **Job Object** immediately after
  `cmd.Start()`. Descendants created afterwards are automatically part
  of the job.
- Override `cmd.Cancel` to `TerminateJobObject`, so cancellation kills
  the **entire descendant tree** atomically. This also closes the
  inherited pipe handles, so `cmd.Wait()` can return.
- Set `cmd.WaitDelay` (10s) as a safety net: once the process has
  exited, Wait force-closes the pipes and returns rather than blocking
  forever — covering the case where the job-object setup fails (e.g.
  nested-job restrictions), in which we fall back to the previous
  single-process kill.
- The Job Object is created **without** `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`,
  so closing the handle on normal completion does not kill legitimate
  background processes; the tree is only torn down on explicit cancel.

Implemented behind `runtime.GOOS == "windows"` with a Windows-only
`processKiller` (Job Object) and no-op stubs elsewhere, so non-Windows
behaviour (default cancellation + `Setpgid`) is unchanged.

## Changes

- `act/container/process_windows.go` — Job Object `processKiller`
  (create / assign / terminate).
- `act/container/process_other.go` — no-op stubs (`//go:build !windows`).
- `act/container/host_environment.go` — wire `cmd.Cancel` (tree kill)
  and `cmd.WaitDelay` into `exec()`.
- `go.mod` / `go.sum` — promote `golang.org/x/sys` to a direct
  dependency.

## Testing

I fully tested it already

## Notes

Follow-up to the Windows leftover-process reaping in #996: that sweep
now actually runs on cancellation because the step no longer hangs
before reaching it.

Reviewed-on: https://gitea.com/gitea/runner/pulls/1011
Reviewed-by: techknowlogick <9+techknowlogick@noreply.gitea.com>
2026-06-02 16:53:27 +00:00
silverwind
f17b6b9fc3 fix(container): re-validate cached container id before reuse (#1003)
`containerReference.id` was cached from `Create()` and never re-validated, so a container torn down out-of-band (AutoRemove on an unexpected exit, daemon-side cleanup, sibling-job race in a parallel matrix) left a stale id behind. The next `Copy`/`Exec` then hit the daemon with that dead id and failed the otherwise-successful job with `Could not find the file /var/run/act/ in container <id>`.

`find()` now `ContainerInspect`s the cached id and clears it only on a definitive `NotFound`; transient errors trust the cache so cleanup pipelines don't abort on a daemon blip. Operations that need a live container (`copyContent`/`copyDir`/`CopyTarStream`/`exec`/`GetContainerArchive`) fail fast with a clear `container "<name>" does not exist` instead of the daemon's generic empty-id error.

---
This PR was written with the help of Claude Opus 4.7

---------

Co-authored-by: Nicolas <bircni@icloud.com>
Reviewed-on: https://gitea.com/gitea/runner/pulls/1003
Reviewed-by: Nicolas <bircni@icloud.com>
Co-authored-by: silverwind <2021+silverwind@noreply.gitea.com>
Co-committed-by: silverwind <2021+silverwind@noreply.gitea.com>
2026-05-29 22:33:44 +00:00
Christopher Homberger
c7c4bd600a fix: support multiline secret masking (#1001)
* command logging exposes multiline secrets more often than before
* duplicated add-mask command in reporter now handles this as well

Closes #998
Co-authored-by: silverwind <2021+silverwind@noreply.gitea.com>
Co-authored-by: silverwind <me@silverwind.io>
Reviewed-on: https://gitea.com/gitea/runner/pulls/1001
Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com>
Reviewed-by: silverwind <2021+silverwind@noreply.gitea.com>
Co-authored-by: Christopher Homberger <christopher.homberger@web.de>
Co-committed-by: Christopher Homberger <christopher.homberger@web.de>
2026-05-29 19:58:15 +00:00
13 changed files with 455 additions and 22 deletions

View File

@@ -26,6 +26,7 @@ import (
"dario.cat/mergo" "dario.cat/mergo"
"github.com/Masterminds/semver" "github.com/Masterminds/semver"
cerrdefs "github.com/containerd/errdefs"
"github.com/docker/cli/cli/compose/loader" "github.com/docker/cli/cli/compose/loader"
"github.com/docker/cli/cli/connhelper" "github.com/docker/cli/cli/connhelper"
"github.com/go-git/go-billy/v5/helper/polyfill" "github.com/go-git/go-billy/v5/helper/polyfill"
@@ -152,6 +153,8 @@ func (cr *containerReference) Copy(destPath string, files ...*FileEntry) common.
func (cr *containerReference) CopyDir(destPath, srcPath string, useGitIgnore bool) common.Executor { func (cr *containerReference) CopyDir(destPath, srcPath string, useGitIgnore bool) common.Executor {
return common.NewPipelineExecutor( return common.NewPipelineExecutor(
common.NewInfoExecutor("docker cp src=%s dst=%s", srcPath, destPath), common.NewInfoExecutor("docker cp src=%s dst=%s", srcPath, destPath),
cr.connect(),
cr.find(),
cr.copyDir(destPath, srcPath, useGitIgnore), cr.copyDir(destPath, srcPath, useGitIgnore),
func(ctx context.Context) error { func(ctx context.Context) error {
// If this fails, then folders have wrong permissions on non root container // If this fails, then folders have wrong permissions on non root container
@@ -167,6 +170,16 @@ func (cr *containerReference) GetContainerArchive(ctx context.Context, srcPath s
if common.Dryrun(ctx) { if common.Dryrun(ctx) {
return nil, errors.New("DRYRUN is not supported in GetContainerArchive") return nil, errors.New("DRYRUN is not supported in GetContainerArchive")
} }
// Direct entry point (no pipeline) — revalidate cr.id ourselves.
if err := cr.connect()(ctx); err != nil {
return nil, err
}
if err := cr.find()(ctx); err != nil {
return nil, err
}
if cr.id == "" {
return nil, cr.missingContainerError("get archive %s", srcPath)
}
result, err := cr.cli.CopyFromContainer(ctx, cr.id, client.CopyFromContainerOptions{SourcePath: srcPath}) result, err := cr.cli.CopyFromContainer(ctx, cr.id, client.CopyFromContainerOptions{SourcePath: srcPath})
if err != nil { if err != nil {
return nil, err return nil, err
@@ -314,10 +327,22 @@ func (cr *containerReference) Close() common.Executor {
} }
} }
// missingContainerError is the shared "container X does not exist" error
// used by ops that need a live cr.id.
func (cr *containerReference) missingContainerError(format string, args ...any) error {
return fmt.Errorf("container %q does not exist; cannot "+format, append([]any{cr.input.Name}, args...)...)
}
func (cr *containerReference) find() common.Executor { func (cr *containerReference) find() common.Executor {
return func(ctx context.Context) error { return func(ctx context.Context) error {
if cr.id != "" { if cr.id != "" {
return nil // Validate cached id; clear only on definitive NotFound so a
// transient daemon error doesn't abort cleanup pipelines.
_, err := cr.cli.ContainerInspect(ctx, cr.id, client.ContainerInspectOptions{})
if !cerrdefs.IsNotFound(err) {
return nil
}
cr.id = ""
} }
containers, err := cr.cli.ContainerList(ctx, client.ContainerListOptions{ containers, err := cr.cli.ContainerList(ctx, client.ContainerListOptions{
All: true, All: true,
@@ -335,7 +360,6 @@ func (cr *containerReference) find() common.Executor {
} }
} }
cr.id = ""
return nil return nil
} }
} }
@@ -592,6 +616,9 @@ func (cr *containerReference) extractFromImageEnv(env *map[string]string) common
func (cr *containerReference) exec(cmd []string, env map[string]string, user, workdir string) common.Executor { func (cr *containerReference) exec(cmd []string, env map[string]string, user, workdir string) common.Executor {
return func(ctx context.Context) error { return func(ctx context.Context) error {
if cr.id == "" {
return cr.missingContainerError("exec %v", cmd)
}
logger := common.Logger(ctx) logger := common.Logger(ctx)
// Fix slashes when running on Windows // Fix slashes when running on Windows
if runtime.GOOS == "windows" { if runtime.GOOS == "windows" {
@@ -746,6 +773,9 @@ func (cr *containerReference) waitForCommand(ctx context.Context, isTerminal boo
} }
func (cr *containerReference) CopyTarStream(ctx context.Context, destPath string, tarStream io.Reader) error { func (cr *containerReference) CopyTarStream(ctx context.Context, destPath string, tarStream io.Reader) error {
if cr.id == "" {
return cr.missingContainerError("copy to %s", destPath)
}
// Mkdir // Mkdir
buf := &bytes.Buffer{} buf := &bytes.Buffer{}
tw := tar.NewWriter(buf) tw := tar.NewWriter(buf)
@@ -779,6 +809,9 @@ func (cr *containerReference) CopyTarStream(ctx context.Context, destPath string
func (cr *containerReference) copyDir(dstPath, srcPath string, useGitIgnore bool) common.Executor { func (cr *containerReference) copyDir(dstPath, srcPath string, useGitIgnore bool) common.Executor {
return func(ctx context.Context) error { return func(ctx context.Context) error {
if cr.id == "" {
return cr.missingContainerError("copy directory to %s", dstPath)
}
logger := common.Logger(ctx) logger := common.Logger(ctx)
tarFile, err := os.CreateTemp("", "act") tarFile, err := os.CreateTemp("", "act")
if err != nil { if err != nil {
@@ -853,6 +886,9 @@ func (cr *containerReference) copyDir(dstPath, srcPath string, useGitIgnore bool
func (cr *containerReference) copyContent(dstPath string, files ...*FileEntry) common.Executor { func (cr *containerReference) copyContent(dstPath string, files ...*FileEntry) common.Executor {
return func(ctx context.Context) error { return func(ctx context.Context) error {
if cr.id == "" {
return cr.missingContainerError("copy to %s", dstPath)
}
logger := common.Logger(ctx) logger := common.Logger(ctx)
var buf bytes.Buffer var buf bytes.Buffer
tw := tar.NewWriter(&buf) tw := tar.NewWriter(&buf)

View File

@@ -19,6 +19,7 @@ import (
"gitea.com/gitea/runner/act/common" "gitea.com/gitea/runner/act/common"
cerrdefs "github.com/containerd/errdefs"
"github.com/moby/moby/api/types/container" "github.com/moby/moby/api/types/container"
mobyclient "github.com/moby/moby/client" mobyclient "github.com/moby/moby/client"
"github.com/sirupsen/logrus/hooks/test" "github.com/sirupsen/logrus/hooks/test"
@@ -98,6 +99,16 @@ func (m *mockDockerClient) CopyToContainer(ctx context.Context, id string, optio
return args.Get(0).(mobyclient.CopyToContainerResult), args.Error(1) return args.Get(0).(mobyclient.CopyToContainerResult), args.Error(1)
} }
func (m *mockDockerClient) ContainerInspect(ctx context.Context, id string, opts mobyclient.ContainerInspectOptions) (mobyclient.ContainerInspectResult, error) {
args := m.Called(ctx, id, opts)
return args.Get(0).(mobyclient.ContainerInspectResult), args.Error(1)
}
func (m *mockDockerClient) ContainerList(ctx context.Context, opts mobyclient.ContainerListOptions) (mobyclient.ContainerListResult, error) {
args := m.Called(ctx, opts)
return args.Get(0).(mobyclient.ContainerListResult), args.Error(1)
}
type endlessReader struct { type endlessReader struct {
io.Reader io.Reader
} }
@@ -298,6 +309,105 @@ func TestDockerCopyTarStreamErrorInMkdir(t *testing.T) {
client.AssertExpectations(t) client.AssertExpectations(t)
} }
// find() must drop a stale cached id so later Copy/Exec don't hit the
// daemon with a torn-down container.
func TestFindRevalidatesStaleID(t *testing.T) {
ctx := context.Background()
notFound := cerrdefs.ErrNotFound.WithMessage("No such container")
boom := errors.New("daemon unreachable")
newCR := func(id string) (*containerReference, *mockDockerClient) {
client := &mockDockerClient{}
return &containerReference{id: id, cli: client, input: &NewContainerInput{Name: "job-1"}}, client
}
listOpts := mobyclient.ContainerListOptions{All: true}
inspectOpts := mobyclient.ContainerInspectOptions{}
t.Run("stale id cleared, name lookup empty", func(t *testing.T) {
cr, client := newCR("stale")
client.On("ContainerInspect", ctx, "stale", inspectOpts).Return(mobyclient.ContainerInspectResult{}, notFound)
client.On("ContainerList", ctx, listOpts).Return(mobyclient.ContainerListResult{}, nil)
require.NoError(t, cr.find()(ctx))
assert.Empty(t, cr.id)
client.AssertExpectations(t)
})
t.Run("stale id cleared, name lookup repopulates", func(t *testing.T) {
cr, client := newCR("stale")
client.On("ContainerInspect", ctx, "stale", inspectOpts).Return(mobyclient.ContainerInspectResult{}, notFound)
client.On("ContainerList", ctx, listOpts).Return(mobyclient.ContainerListResult{Items: []container.Summary{
{ID: "other", Names: []string{"/somebody-else"}},
{ID: "fresh", Names: []string{"/job-1"}},
}}, nil)
require.NoError(t, cr.find()(ctx))
assert.Equal(t, "fresh", cr.id)
client.AssertExpectations(t)
})
t.Run("live id kept", func(t *testing.T) {
cr, client := newCR("live")
client.On("ContainerInspect", ctx, "live", inspectOpts).Return(mobyclient.ContainerInspectResult{}, nil)
require.NoError(t, cr.find()(ctx))
assert.Equal(t, "live", cr.id)
client.AssertExpectations(t)
})
t.Run("transient inspect error trusts cache", func(t *testing.T) {
cr, client := newCR("live")
client.On("ContainerInspect", ctx, "live", inspectOpts).Return(mobyclient.ContainerInspectResult{}, boom)
require.NoError(t, cr.find()(ctx))
assert.Equal(t, "live", cr.id)
client.AssertExpectations(t)
})
t.Run("list error propagates", func(t *testing.T) {
cr, client := newCR("")
client.On("ContainerList", ctx, listOpts).Return(mobyclient.ContainerListResult{}, boom)
require.ErrorIs(t, cr.find()(ctx), boom)
client.AssertExpectations(t)
})
}
// Every daemon entry point fails fast with a clear, container-named
// error when no live cr.id is known.
func TestRejectsMissingContainer(t *testing.T) {
ctx := context.Background()
client := &mockDockerClient{}
client.On("ContainerList", ctx, mobyclient.ContainerListOptions{All: true}).Return(mobyclient.ContainerListResult{}, nil)
cr := &containerReference{cli: client, input: &NewContainerInput{Name: "job-1"}}
check := func(op string, err error) {
t.Helper()
require.Error(t, err, op)
assert.Contains(t, err.Error(), `container "job-1" does not exist`, op)
}
check("copyContent", cr.copyContent("/var/run/act", &FileEntry{Name: "x", Mode: 0o644})(ctx))
check("copyDir", cr.copyDir("/var/run/act", "/src", false)(ctx))
check("CopyTarStream", cr.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{}))
check("exec", cr.exec([]string{"echo"}, nil, "", "")(ctx))
_, err := cr.GetContainerArchive(ctx, "/var/run/act/x")
check("GetContainerArchive", err)
}
// End-to-end: a stale cr.id is cleared, repopulated from name lookup,
// and the Copy completes against the fresh id.
func TestPublicCopyPipelineHandlesStaleID(t *testing.T) {
ctx := context.Background()
client := &mockDockerClient{}
client.On("ContainerInspect", ctx, "stale", mobyclient.ContainerInspectOptions{}).
Return(mobyclient.ContainerInspectResult{}, cerrdefs.ErrNotFound.WithMessage("gone"))
client.On("ContainerList", ctx, mobyclient.ContainerListOptions{All: true}).
Return(mobyclient.ContainerListResult{Items: []container.Summary{
{ID: "fresh", Names: []string{"/job-1"}},
}}, nil)
client.On("CopyToContainer", ctx, "fresh", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
return opts.DestinationPath == "/var/run/act"
})).Return(mobyclient.CopyToContainerResult{}, nil)
cr := &containerReference{id: "stale", cli: client, input: &NewContainerInput{Name: "job-1"}}
require.NoError(t, cr.Copy("/var/run/act", &FileEntry{Name: "x", Mode: 0o644})(ctx))
assert.Equal(t, "fresh", cr.id)
client.AssertExpectations(t)
}
// TestDockerCopyToSymlinkPath is a regression test for gitea/runner#981. Most base images // TestDockerCopyToSymlinkPath is a regression test for gitea/runner#981. Most base images
// symlink /var/run to /run, so copying into /var/run/act traverses that symlink. The broken // symlink /var/run to /run, so copying into /var/run/act traverses that symlink. The broken
// docker 29.5.1 daemon fails the extraction with "mkdirat var/run: file exists" (fixed in // docker 29.5.1 daemon fails the extraction with "mkdirat var/run: file exists" (fixed in

View File

@@ -322,6 +322,30 @@ func (e *HostEnvironment) exec(ctx context.Context, command []string, cmdline st
cmd.Stderr = e.StdOut cmd.Stderr = e.StdOut
cmd.Dir = wd cmd.Dir = wd
cmd.SysProcAttr = getSysProcAttr(cmdline, false) cmd.SysProcAttr = getSysProcAttr(cmdline, false)
// On Windows a step often launches a process tree (a shell that starts a
// child which spawns further GUI or background processes). The default
// context cancellation only kills the direct child, leaving the rest of the
// tree running; and because the orphans inherit cmd's stdout/stderr pipe,
// cmd.Wait() would block forever, hanging the runner. Kill the whole tree
// via a Job Object on cancellation, and bound the wait so a leftover pipe
// writer can never hang Wait indefinitely.
var killer atomic.Pointer[processKiller]
if runtime.GOOS == "windows" {
cmd.Cancel = func() error {
if k := killer.Load(); k != nil {
return k.Kill()
}
if cmd.Process != nil {
return cmd.Process.Kill()
}
return nil
}
// Once the step process has exited, give its I/O pipes at most this long
// to drain before Wait force-closes them and returns (Go's WaitDelay).
cmd.WaitDelay = 10 * time.Second
}
var ppty *os.File var ppty *os.File
var tty *os.File var tty *os.File
defer func() { defer func() {
@@ -351,6 +375,18 @@ func (e *HostEnvironment) exec(ctx context.Context, command []string, cmdline st
if err := cmd.Start(); err != nil { if err := cmd.Start(); err != nil {
return err return err
} }
if runtime.GOOS == "windows" {
// Assign the started process to a Job Object so cmd.Cancel can kill the
// whole descendant tree. Children spawned afterwards are auto-included.
// On failure (e.g. nested-job restrictions) we fall back to the default
// single-process kill; WaitDelay + end-of-job cleanup still apply.
if k, kerr := newProcessKiller(cmd.Process); kerr != nil {
common.Logger(ctx).Warnf("process tree kill setup failed, falling back to single-process kill: %v", kerr)
} else {
killer.Store(k)
defer k.Close()
}
}
err = cmd.Wait() err = cmd.Wait()
if err != nil { if err != nil {
var exitErr *exec.ExitError var exitErr *exec.ExitError

View File

@@ -0,0 +1,19 @@
// Copyright 2026 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
//go:build !windows
package container
import "os"
// processKiller is a no-op on non-Windows platforms. The Job Object based
// tree-kill is only wired in on Windows (see exec()); elsewhere the default
// exec.CommandContext cancellation and Setpgid handling apply.
type processKiller struct{}
func newProcessKiller(_ *os.Process) (*processKiller, error) { return &processKiller{}, nil }
func (k *processKiller) Kill() error { return nil }
func (k *processKiller) Close() error { return nil }

View File

@@ -0,0 +1,71 @@
// Copyright 2026 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package container
import (
"os"
"golang.org/x/sys/windows"
)
// processKiller terminates a step process together with its entire descendant
// tree via a Windows Job Object.
//
// Background: a step often launches a process tree (a shell that starts a
// child which in turn spawns further GUI or background processes). The default
// exec.CommandContext cancellation only kills the direct child, so cancelling a
// job left the rest of the tree running. Because those orphans inherited the
// step's stdout/stderr pipe, cmd.Wait() also blocked forever and the runner hung.
//
// Assigning the step process to a Job Object lets us kill the whole tree
// atomically on cancellation (TerminateJobObject), which also closes the
// inherited pipe handles so cmd.Wait() can return.
type processKiller struct {
job windows.Handle
}
// newProcessKiller creates a Job Object and assigns p (an already-started
// process) to it. Children spawned by p afterwards are automatically part of
// the job. The job does NOT use JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, so closing
// the handle on normal completion does not kill legitimate background
// processes; the tree is only torn down by an explicit Kill (cancellation).
func newProcessKiller(p *os.Process) (*processKiller, error) {
job, err := windows.CreateJobObject(nil, nil)
if err != nil {
return nil, err
}
h, err := windows.OpenProcess(windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, false, uint32(p.Pid))
if err != nil {
windows.CloseHandle(job)
return nil, err
}
defer windows.CloseHandle(h)
if err := windows.AssignProcessToJobObject(job, h); err != nil {
windows.CloseHandle(job)
return nil, err
}
return &processKiller{job: job}, nil
}
// Kill terminates every process currently assigned to the job (the step process
// and all of its descendants).
func (k *processKiller) Kill() error {
if k == nil || k.job == 0 {
return nil
}
return windows.TerminateJobObject(k.job, 1)
}
// Close releases the job handle. It does not terminate the processes.
func (k *processKiller) Close() error {
if k == nil || k.job == 0 {
return nil
}
h := k.job
k.job = 0
return windows.CloseHandle(h)
}

View File

@@ -0,0 +1,78 @@
// Copyright 2026 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package container
import (
"fmt"
"os"
"os/exec"
"path/filepath"
"strconv"
"strings"
"testing"
"time"
"github.com/stretchr/testify/require"
"golang.org/x/sys/windows"
)
// processAlive reports whether pid refers to a still-running process.
func processAlive(pid int) bool {
h, err := windows.OpenProcess(windows.PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(pid))
if err != nil {
return false
}
defer windows.CloseHandle(h)
var code uint32
if err := windows.GetExitCodeProcess(h, &code); err != nil {
return false
}
const stillActive = 259 // STILL_ACTIVE
return code == stillActive
}
// TestProcessKillerKillsTree verifies that a process assigned to the Job Object
// is terminated together with a child it spawns afterwards. This mirrors a step
// that launches a child which spawns further processes, where cancelling the
// job must take down the whole tree, not just the direct child.
func TestProcessKillerKillsTree(t *testing.T) {
dir := t.TempDir()
pidFile := filepath.Join(dir, "child.pid")
// Parent powershell spawns a detached, long-lived child powershell (writing
// its PID to a file) and then sleeps. The child is launched AFTER the parent
// has been assigned to the job, so it must be captured by the job too.
script := fmt.Sprintf(
`$c = Start-Process powershell -PassThru -ArgumentList '-NoProfile','-Command','Start-Sleep -Seconds 600'; `+
`Set-Content -LiteralPath %q -Value $c.Id; Start-Sleep -Seconds 600`, pidFile)
cmd := exec.Command("powershell.exe", "-NoProfile", "-Command", script)
require.NoError(t, cmd.Start())
t.Cleanup(func() { _ = cmd.Process.Kill() })
killer, err := newProcessKiller(cmd.Process)
require.NoError(t, err)
defer killer.Close()
// Wait for the child PID to be reported.
var childPID int
require.Eventually(t, func() bool {
b, e := os.ReadFile(pidFile)
if e != 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")
// Killing the job must terminate both the parent and the detached child.
require.NoError(t, killer.Kill())
require.Eventually(t, func() bool {
return !processAlive(cmd.Process.Pid) && !processAlive(childPID)
}, 20*time.Second, 200*time.Millisecond, "parent and child should both be terminated")
}

View File

@@ -51,7 +51,7 @@ func (rc *RunContext) commandHandler(ctx context.Context) common.LineHandler {
logger.Infof("%s", line) logger.Infof("%s", line)
return false return false
} }
arg = unescapeCommandData(arg) arg = UnescapeCommandData(arg)
kvPairs = unescapeKvPairs(kvPairs) kvPairs = unescapeKvPairs(kvPairs)
switch command { switch command {
case "set-env": case "set-env":
@@ -151,7 +151,7 @@ func parseKeyValuePairs(kvPairs, separator string) map[string]string {
return rtn return rtn
} }
func unescapeCommandData(arg string) string { func UnescapeCommandData(arg string) string {
escapeMap := map[string]string{ escapeMap := map[string]string{
"%25": "%", "%25": "%",
"%0D": "\r", "%0D": "\r",

View File

@@ -10,6 +10,7 @@ import (
"fmt" "fmt"
"io" "io"
"os" "os"
"slices"
"strings" "strings"
"sync" "sync"
@@ -166,9 +167,29 @@ func withStepLogger(ctx context.Context, stepNumber int, stepID, stepName, stage
type entryProcessor func(entry *logrus.Entry) *logrus.Entry type entryProcessor func(entry *logrus.Entry) *logrus.Entry
func AppendSecretMasker(oldnew []string, v string) []string {
ret := oldnew
for l := range strings.SplitSeq(v, "\n") {
tm := strings.TrimSpace(l)
// formatted JSON secrets could otherwise mask {,[,],} everywhere
if len(tm) > 1 {
ret = append(ret, tm, "***")
}
}
return ret
}
// valueMasker applies secrets and ::add-mask:: patterns to every log entry, including // valueMasker applies secrets and ::add-mask:: patterns to every log entry, including
// raw_output (command/stream) lines; there is no bypass by field. // raw_output (command/stream) lines; there is no bypass by field.
func valueMasker(insecureSecrets bool, secrets map[string]string) entryProcessor { func valueMasker(insecureSecrets bool, secrets map[string]string) entryProcessor {
var oldnew []string
for _, v := range secrets {
oldnew = AppendSecretMasker(oldnew, v)
}
oldnew = slices.Clip(oldnew)
defReplacer := strings.NewReplacer(oldnew...)
return func(entry *logrus.Entry) *logrus.Entry { return func(entry *logrus.Entry) *logrus.Entry {
if insecureSecrets { if insecureSecrets {
return entry return entry
@@ -176,16 +197,16 @@ func valueMasker(insecureSecrets bool, secrets map[string]string) entryProcessor
masks := Masks(entry.Context) masks := Masks(entry.Context)
for _, v := range secrets { if len(*masks) == 0 {
if v != "" { entry.Message = defReplacer.Replace(entry.Message)
entry.Message = strings.ReplaceAll(entry.Message, v, "***") } else {
} cmasker := oldnew
}
for _, v := range *masks { for _, v := range *masks {
if v != "" { cmasker = AppendSecretMasker(cmasker, v)
entry.Message = strings.ReplaceAll(entry.Message, v, "***")
} }
entry.Message = strings.NewReplacer(cmasker...).Replace(entry.Message)
} }
return entry return entry

52
act/runner/logger_test.go Normal file
View File

@@ -0,0 +1,52 @@
// Copyright 2026 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package runner
import (
"strings"
"testing"
"github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
)
func TestValueMasker(t *testing.T) {
table := []struct {
name string
lines string
secrets map[string]string
masks []string
disallowed []string
}{
{
name: "Multiline Private Key",
lines: "cat << EOF > private.key\nPRIVATE_KEY_BEGIN\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\nPRIVATE_KEY_END\nEOF",
secrets: map[string]string{
"PRIVATE_KEY": "PRIVATE_KEY_BEGIN\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\nPRIVATE_KEY_END",
},
disallowed: []string{"KEY", "dsdfseffefsefes", "PRIVATE_KEY_END"},
},
{
name: "Multiline Private Key in masks",
lines: "cat << EOF > private.key\nPRIVATE_KEY_BEGIN\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\nPRIVATE_KEY_END\nEOF",
masks: []string{"PRIVATE_KEY_BEGIN\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\ndsdfseffefsefes\nPRIVATE_KEY_END"},
disallowed: []string{"KEY", "dsdfseffefsefes", "PRIVATE_KEY_END"},
},
}
for _, entry := range table {
t.Run(entry.name, func(t *testing.T) {
ctx := WithMasks(t.Context(), &entry.masks)
masker := valueMasker(false, entry.secrets)
for line := range strings.SplitSeq(entry.lines, "\n") {
lentry := masker(&logrus.Entry{
Context: ctx,
Message: line,
})
for _, line := range entry.disallowed {
assert.NotContains(t, lentry.Message, line)
}
}
})
}
}

2
go.mod
View File

@@ -37,6 +37,7 @@ require (
github.com/timshannon/bolthold v0.0.0-20240314194003-30aac6950928 github.com/timshannon/bolthold v0.0.0-20240314194003-30aac6950928
go.etcd.io/bbolt v1.4.3 go.etcd.io/bbolt v1.4.3
go.yaml.in/yaml/v4 v4.0.0-rc.3 go.yaml.in/yaml/v4 v4.0.0-rc.3
golang.org/x/sys v0.44.0
golang.org/x/term v0.43.0 golang.org/x/term v0.43.0
google.golang.org/protobuf v1.36.11 google.golang.org/protobuf v1.36.11
gotest.tools/v3 v3.5.2 gotest.tools/v3 v3.5.2
@@ -106,7 +107,6 @@ require (
golang.org/x/crypto v0.50.0 // indirect golang.org/x/crypto v0.50.0 // indirect
golang.org/x/net v0.53.0 // indirect golang.org/x/net v0.53.0 // indirect
golang.org/x/sync v0.20.0 // indirect golang.org/x/sync v0.20.0 // indirect
golang.org/x/sys v0.44.0 // indirect
gopkg.in/warnings.v0 v0.1.2 // indirect gopkg.in/warnings.v0 v0.1.2 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect
) )

4
go.sum
View File

@@ -1,7 +1,5 @@
code.gitea.io/actions-proto-go v0.4.1 h1:l0EYhjsgpUe/1VABo2eK7zcoNX2W44WOnb0MSLrKfls= code.gitea.io/actions-proto-go v0.4.1 h1:l0EYhjsgpUe/1VABo2eK7zcoNX2W44WOnb0MSLrKfls=
code.gitea.io/actions-proto-go v0.4.1/go.mod h1:mn7Wkqz6JbnTOHQpot3yDeHx+O5C9EGhMEE+htvHBas= code.gitea.io/actions-proto-go v0.4.1/go.mod h1:mn7Wkqz6JbnTOHQpot3yDeHx+O5C9EGhMEE+htvHBas=
connectrpc.com/connect v1.19.2 h1:McQ83FGdzL+t60peksi0gXC7MQ/iLKgLduAnThbM0mo=
connectrpc.com/connect v1.19.2/go.mod h1:tN20fjdGlewnSFeZxLKb0xwIZ6ozc3OQs2hTXy4du9w=
connectrpc.com/connect v1.20.0 h1:6TNDAB+WeNd2uolWNlYczB5E0KNNaVMNUEx8JEUsPmQ= connectrpc.com/connect v1.20.0 h1:6TNDAB+WeNd2uolWNlYczB5E0KNNaVMNUEx8JEUsPmQ=
connectrpc.com/connect v1.20.0/go.mod h1:A2ygJrukXwWy32vkCAAHNVguZrqZ+jeZ9rGRnGR4dN4= connectrpc.com/connect v1.20.0/go.mod h1:A2ygJrukXwWy32vkCAAHNVguZrqZ+jeZ9rGRnGR4dN4=
cyphar.com/go-pathrs v0.2.3 h1:0pH8gep37wB0BgaXrEaN1OtZhUMeS7VvaejSr6i822o= cyphar.com/go-pathrs v0.2.3 h1:0pH8gep37wB0BgaXrEaN1OtZhUMeS7VvaejSr6i822o=
@@ -149,8 +147,6 @@ github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8
github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM= github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM=
github.com/opencontainers/image-spec v1.1.1 h1:y0fUlFfIZhPF1W537XOLg0/fcx6zcHCJwooC2xJA040= github.com/opencontainers/image-spec v1.1.1 h1:y0fUlFfIZhPF1W537XOLg0/fcx6zcHCJwooC2xJA040=
github.com/opencontainers/image-spec v1.1.1/go.mod h1:qpqAh3Dmcf36wStyyWU+kCeDgrGnAve2nCC8+7h8Q0M= github.com/opencontainers/image-spec v1.1.1/go.mod h1:qpqAh3Dmcf36wStyyWU+kCeDgrGnAve2nCC8+7h8Q0M=
github.com/opencontainers/selinux v1.14.1 h1:a7XlXV/nN/l5zFP1FWZYoExpClu1QOPMfWUV2CZ8kEQ=
github.com/opencontainers/selinux v1.14.1/go.mod h1:LenyElirjUHszfxrjuFqC85HIeXZKumHcKMQtnaDlQQ=
github.com/opencontainers/selinux v1.15.0 h1:4Gs40e/R2FvM8PC1HPaPncLLaDor8Y2WDfk5gjU9o5M= github.com/opencontainers/selinux v1.15.0 h1:4Gs40e/R2FvM8PC1HPaPncLLaDor8Y2WDfk5gjU9o5M=
github.com/opencontainers/selinux v1.15.0/go.mod h1:LenyElirjUHszfxrjuFqC85HIeXZKumHcKMQtnaDlQQ= github.com/opencontainers/selinux v1.15.0/go.mod h1:LenyElirjUHszfxrjuFqC85HIeXZKumHcKMQtnaDlQQ=
github.com/pjbgf/sha1cd v0.6.0 h1:3WJ8Wz8gvDz29quX1OcEmkAlUg9diU4GxJHqs0/XiwU= github.com/pjbgf/sha1cd v0.6.0 h1:3WJ8Wz8gvDz29quX1OcEmkAlUg9diU4GxJHqs0/XiwU=

View File

@@ -13,6 +13,7 @@ import (
"sync/atomic" "sync/atomic"
"time" "time"
"gitea.com/gitea/runner/act/runner"
"gitea.com/gitea/runner/internal/pkg/client" "gitea.com/gitea/runner/internal/pkg/client"
"gitea.com/gitea/runner/internal/pkg/config" "gitea.com/gitea/runner/internal/pkg/config"
"gitea.com/gitea/runner/internal/pkg/metrics" "gitea.com/gitea/runner/internal/pkg/metrics"
@@ -73,13 +74,13 @@ type Reporter struct {
func NewReporter(ctx context.Context, cancel context.CancelFunc, client client.Client, task *runnerv1.Task, cfg *config.Config) *Reporter { func NewReporter(ctx context.Context, cancel context.CancelFunc, client client.Client, task *runnerv1.Task, cfg *config.Config) *Reporter {
var oldnew []string var oldnew []string
if v := task.Context.Fields["token"].GetStringValue(); v != "" { if v := task.Context.Fields["token"].GetStringValue(); v != "" {
oldnew = append(oldnew, v, "***") oldnew = runner.AppendSecretMasker(oldnew, v)
} }
if v := task.Context.Fields["gitea_runtime_token"].GetStringValue(); v != "" { if v := task.Context.Fields["gitea_runtime_token"].GetStringValue(); v != "" {
oldnew = append(oldnew, v, "***") oldnew = runner.AppendSecretMasker(oldnew, v)
} }
for _, v := range task.Secrets { for _, v := range task.Secrets {
oldnew = append(oldnew, v, "***") oldnew = runner.AppendSecretMasker(oldnew, v)
} }
rv := &Reporter{ rv := &Reporter{
@@ -689,7 +690,7 @@ func (r *Reporter) parseLogRow(entry *log.Entry) *runnerv1.LogRow {
matches := cmdRegex.FindStringSubmatch(content) matches := cmdRegex.FindStringSubmatch(content)
if matches != nil { if matches != nil {
if output := r.handleCommand(content, matches[1], matches[3]); output != nil { if output := r.handleCommand(content, matches[1], runner.UnescapeCommandData(matches[3])); output != nil {
content = *output content = *output
} else { } else {
return nil return nil
@@ -705,6 +706,6 @@ func (r *Reporter) parseLogRow(entry *log.Entry) *runnerv1.LogRow {
} }
func (r *Reporter) addMask(msg string) { func (r *Reporter) addMask(msg string) {
r.oldnew = append(r.oldnew, msg, "***") r.oldnew = runner.AppendSecretMasker(r.oldnew, msg)
r.logReplacer = strings.NewReplacer(r.oldnew...) r.logReplacer = strings.NewReplacer(r.oldnew...)
} }

View File

@@ -50,6 +50,19 @@ func TestReporter_parseLogRow(t *testing.T) {
"foo *** bar", "foo *** bar",
}, },
}, },
{
"Add-mask-multiline", false,
[]string{
"foo mysecret bar",
"::add-mask::LINE1%0ALINE2",
"foo LINE1 bar",
},
[]string{
"foo mysecret bar",
"<nil>",
"foo *** bar",
},
},
{ {
"Debug enabled", true, "Debug enabled", true,
[]string{ []string{