From eeb479ea89a54e42a8ede6da9b00e5c1da98fca7 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Wed, 1 Jul 2026 18:50:34 +0000 Subject: [PATCH] fix: prevent exponential growth of RunContext masks in composite actions (#1059) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A composite action's `RunContext` is **seeded with its parent's `Masks` slice** in `newCompositeRunContext`. After the composite finished running, the *entire* `compositeRC.Masks` (the seeded parent masks plus any added during the run) was appended straight back into the parent: ```go rc.Masks = append(rc.Masks, compositeRC.Masks...) ``` Because the composite already contained the parent's masks, every composite action roughly **doubled** the parent's `Masks` length. With many nested/repeated composite actions this grows exponentially: ``` before=1152 after=2304 compositeLen=1152 before=2304 after=4608 compositeLen=2304 before=4608 after=9216 compositeLen=4608 ... ``` ### Impact The bloated `Masks` slice made per-log-line secret redaction (`strings.makeGenericReplacer`, rebuilt per line since #1001) extremely slow. Log writes fell behind enough to keep subprocess pipes open after the process had already exited, surfacing as seemingly random CI failures — most often in later steps of mask-heavy workflows: ``` Error: exec: WaitDelay expired before I/O complete ``` The split of `cmd.Run()` into `cmd.Start()` + `cmd.Wait()` (#883) is what turned this long-standing latency into a hard failure. ### Fix Only append masks that aren't already present in the parent, via a small `appendUniqueMasks` helper. This keeps the mask count bounded by the number of *distinct* secrets instead of growing exponentially. I kept `Masks` as a `[]string` rather than switching to a set type, since it's passed by pointer to loggers across `logger.go`, `run_context.go`, and `runner.go` — a type change would be a much wider, riskier refactor. The `O(n·m)` dedup is harmless precisely because `n` now stays small. Fixes #1054Reviewed-on: https://gitea.com/gitea/runner/pulls/1059 Reviewed-by: Lunny Xiao --- act/runner/action_composite.go | 20 ++++++++- act/runner/action_composite_test.go | 70 +++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 act/runner/action_composite_test.go diff --git a/act/runner/action_composite.go b/act/runner/action_composite.go index 15d7b67c..6dd12acd 100644 --- a/act/runner/action_composite.go +++ b/act/runner/action_composite.go @@ -8,6 +8,7 @@ import ( "context" "errors" "regexp" + "slices" "strconv" "strings" @@ -85,6 +86,19 @@ func newCompositeRunContext(ctx context.Context, parent *RunContext, step action return compositerc } +// appendUniqueMasks appends the masks from src to dst, skipping any mask that +// is already present in dst. This prevents the parent RunContext's Masks slice +// from growing exponentially when composite actions are nested or repeated, +// since each composite RunContext is seeded with its parent's masks. +func appendUniqueMasks(dst, src []string) []string { + for _, m := range src { + if !slices.Contains(dst, m) { + dst = append(dst, m) + } + } + return dst +} + func execAsComposite(step actionStep) common.Executor { rc := step.getRunContext() action := step.getActionModel() @@ -110,7 +124,11 @@ func execAsComposite(step actionStep) common.Executor { }, eval.Interpolate(ctx, output.Value)) } - rc.Masks = append(rc.Masks, compositeRC.Masks...) + // compositeRC.Masks is seeded with rc.Masks (see newCompositeRunContext) + // and may have additional masks appended while the composite action runs. + // Only append masks that are not already present, otherwise nested or + // repeated composite actions grow rc.Masks exponentially. + rc.Masks = appendUniqueMasks(rc.Masks, compositeRC.Masks) rc.ExtraPath = compositeRC.ExtraPath // compositeRC.Env is dirty, contains INPUT_ and merged step env, only rely on compositeRC.GlobalEnv mergeIntoMap := mergeIntoMapCaseSensitive diff --git a/act/runner/action_composite_test.go b/act/runner/action_composite_test.go new file mode 100644 index 00000000..c8faeedb --- /dev/null +++ b/act/runner/action_composite_test.go @@ -0,0 +1,70 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package runner + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestAppendUniqueMasks(t *testing.T) { + tests := []struct { + name string + dst []string + src []string + want []string + }{ + { + name: "appends new masks", + dst: []string{"a"}, + src: []string{"b", "c"}, + want: []string{"a", "b", "c"}, + }, + { + name: "skips masks already present", + dst: []string{"a", "b"}, + src: []string{"a", "b"}, + want: []string{"a", "b"}, + }, + { + name: "deduplicates within src", + dst: []string{"a"}, + src: []string{"b", "b", "a"}, + want: []string{"a", "b"}, + }, + { + name: "empty src leaves dst unchanged", + dst: []string{"a"}, + src: nil, + want: []string{"a"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, appendUniqueMasks(tt.dst, tt.src)) + }) + } +} + +// TestAppendUniqueMasksNoExponentialGrowth reproduces the exponential growth of +// the parent's Masks slice observed with nested/repeated composite actions. A +// composite RunContext is seeded with its parent's masks and the whole seeded +// slice was previously appended back into the parent, doubling its length on +// every composite action. +func TestAppendUniqueMasksNoExponentialGrowth(t *testing.T) { + parentMasks := []string{"secret"} + + for range 20 { + // compositeRC.Masks starts as a copy of the parent's masks (it is + // seeded with parent.Masks in newCompositeRunContext). + compositeMasks := make([]string, len(parentMasks)) + copy(compositeMasks, parentMasks) + + parentMasks = appendUniqueMasks(parentMasks, compositeMasks) + } + + assert.Equal(t, []string{"secret"}, parentMasks) +}