From ce6502e7b6a524c5058b1c3836fc6d6f55f3366d Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Wed, 20 Aug 2025 19:56:03 +0000 Subject: [PATCH] chore: fix 'false positive' data race detection in Id/Number default init (#867) A step's `Id` & `Number` are potentially initialized in different goroutines on matrix evaluations; this change ensures they're initialized before execution fans out to multiple goroutines. There doesn't seem to be any functional impact of this data race for end-users. Where `Number` was previously initialized, a runtime error was added to ensure that the behavior is the same. `ID` data race: ``` ================== WARNING: DATA RACE Read at 0x00c0001ff348 by goroutine 77: code.forgejo.org/forgejo/runner/v9/act/runner.newJobExecutor() /.../forgejo-runner/act/runner/job_executor.go:64 +0x424 code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).Executor() /.../forgejo-runner/act/runner/run_context.go:931 +0x2c6 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.1() /.../forgejo-runner/act/runner/runner.go:218 +0x150 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2.1() /.../forgejo-runner/act/common/executor.go:107 +0x61 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2.gowrap1() /.../forgejo-runner/act/common/executor.go:109 +0x4f Previous write at 0x00c0001ff348 by goroutine 76: code.forgejo.org/forgejo/runner/v9/act/runner.newJobExecutor() /.../forgejo-runner/act/runner/job_executor.go:65 +0x4cc code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).Executor() /.../forgejo-runner/act/runner/run_context.go:931 +0x2c6 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.1() /.../forgejo-runner/act/runner/runner.go:218 +0x150 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2.1() /.../forgejo-runner/act/common/executor.go:107 +0x61 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2.gowrap1() /.../forgejo-runner/act/common/executor.go:109 +0x4f Goroutine 77 (running) created at: code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2() /.../forgejo-runner/act/common/executor.go:105 +0x144 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.3.1() /.../forgejo-runner/act/common/executor.go:107 +0x61 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.3.gowrap1() /.../forgejo-runner/act/common/executor.go:109 +0x4f Goroutine 76 (running) created at: code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2() /.../forgejo-runner/act/common/executor.go:105 +0x144 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.3.1() /.../forgejo-runner/act/common/executor.go:107 +0x61 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.3.gowrap1() /.../forgejo-runner/act/common/executor.go:109 +0x4f ================== ``` `Number` data race: ``` ================== WARNING: DATA RACE Write at 0x00c0001ff340 by goroutine 77: code.forgejo.org/forgejo/runner/v9/act/runner.newJobExecutor() /.../forgejo-runner/act/runner/job_executor.go:67 +0x536 code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).Executor() /.../forgejo-runner/act/runner/run_context.go:931 +0x2c6 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.1() /.../forgejo-runner/act/runner/runner.go:218 +0x150 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2.1() /.../forgejo-runner/act/common/executor.go:107 +0x61 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2.gowrap1() /.../forgejo-runner/act/common/executor.go:109 +0x4f Previous write at 0x00c0001ff340 by goroutine 76: code.forgejo.org/forgejo/runner/v9/act/runner.newJobExecutor() /.../forgejo-runner/act/runner/job_executor.go:67 +0x536 code.forgejo.org/forgejo/runner/v9/act/runner.(*RunContext).Executor() /.../forgejo-runner/act/runner/run_context.go:931 +0x2c6 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.1() /.../forgejo-runner/act/runner/runner.go:218 +0x150 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2.1() /.../forgejo-runner/act/common/executor.go:107 +0x61 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2.gowrap1() /.../forgejo-runner/act/common/executor.go:109 +0x4f Goroutine 77 (running) created at: code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2() /.../forgejo-runner/act/common/executor.go:105 +0x144 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.3.1() /.../forgejo-runner/act/common/executor.go:107 +0x61 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.3.gowrap1() /.../forgejo-runner/act/common/executor.go:109 +0x4f Goroutine 76 (running) created at: code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.2() /.../forgejo-runner/act/common/executor.go:105 +0x144 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.3.1() /.../forgejo-runner/act/common/executor.go:107 +0x61 code.forgejo.org/forgejo/runner/v9/act/runner.(*runnerImpl).NewPlanExecutor.func1.NewParallelExecutor.3.gowrap1() /.../forgejo-runner/act/common/executor.go:109 +0x4f ================== ``` - other - [PR](https://code.forgejo.org/forgejo/runner/pulls/867): chore: fix 'false positive' data race detection in Id/Number default init Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/867 Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- act/model/action.go | 16 ++++++++++++++++ act/model/workflow.go | 18 ++++++++++++++++++ act/runner/action_composite.go | 17 ++++++++--------- act/runner/job_executor.go | 10 +++------- act/runner/job_executor_test.go | 9 ++++++--- 5 files changed, 51 insertions(+), 19 deletions(-) diff --git a/act/model/action.go b/act/model/action.go index b174e955..50bbc905 100644 --- a/act/model/action.go +++ b/act/model/action.go @@ -72,6 +72,22 @@ type ActionRuns struct { Steps []Step `yaml:"steps"` } +func (actionRuns *ActionRuns) UnmarshalYAML(value *yaml.Node) error { + type ActionRunsDefault ActionRuns + if err := value.Decode((*ActionRunsDefault)(actionRuns)); err != nil { + return err + } + for i := range actionRuns.Steps { + step := &actionRuns.Steps[i] + // Set `Number` and `ID` on each step based upon their position in the steps array: + if step.ID == "" { + step.ID = fmt.Sprintf("%d", i) + } + step.Number = i + } + return nil +} + // Action describes a metadata file for GitHub actions. The metadata filename must be either action.yml or action.yaml. The data in the metadata file defines the inputs, outputs and main entrypoint for your action. type Action struct { Name string `yaml:"name"` diff --git a/act/model/workflow.go b/act/model/workflow.go index c9acd375..a9a2d32b 100644 --- a/act/model/workflow.go +++ b/act/model/workflow.go @@ -594,6 +594,24 @@ func (j *Job) Type() (JobType, error) { return JobTypeDefault, nil } +func (j *Job) UnmarshalYAML(value *yaml.Node) error { + type JobDefault Job + if err := value.Decode((*JobDefault)(j)); err != nil { + return err + } + for i, step := range j.Steps { + if step == nil { + continue + } + // Set `Number` and `ID` on each step based upon their position in the steps array: + if step.ID == "" { + step.ID = fmt.Sprintf("%d", i) + } + step.Number = i + } + return nil +} + // ContainerSpec is the specification of the container to use for the job type ContainerSpec struct { Image string `yaml:"image"` diff --git a/act/runner/action_composite.go b/act/runner/action_composite.go index fdee793d..45481cd4 100644 --- a/act/runner/action_composite.go +++ b/act/runner/action_composite.go @@ -136,17 +136,16 @@ func (rc *RunContext) compositeExecutor(action *model.Action) *compositeSteps { sf := &stepFactoryImpl{} - for i, step := range action.Runs.Steps { - if step.ID == "" { - step.ID = fmt.Sprintf("%d", i) + for i, stepModel := range action.Runs.Steps { + if stepModel.Number != i { + return &compositeSteps{ + pre: func(ctx context.Context) error { return nil }, + main: common.NewErrorExecutor(fmt.Errorf("internal error: invalid Step: Number expected %v, was actually %v", i, stepModel.Number)), + post: func(ctx context.Context) error { return nil }, + } } - step.Number = i - // create a copy of the step, since this composite action could - // run multiple times and we might modify the instance - stepcopy := step - - step, err := sf.newStep(&stepcopy, rc) + step, err := sf.newStep(&stepModel, rc) if err != nil { return &compositeSteps{ main: common.NewErrorExecutor(err), diff --git a/act/runner/job_executor.go b/act/runner/job_executor.go index 3cb4f206..ebecf31c 100644 --- a/act/runner/job_executor.go +++ b/act/runner/job_executor.go @@ -57,14 +57,10 @@ func newJobExecutor(info jobInfo, sf stepFactory, rc *RunContext) common.Executo for i, stepModel := range infoSteps { if stepModel == nil { - return func(ctx context.Context) error { - return fmt.Errorf("invalid Step %v: missing run or uses key", i) - } + return common.NewErrorExecutor(fmt.Errorf("invalid Step %v: missing run or uses key", i)) + } else if stepModel.Number != i { + return common.NewErrorExecutor(fmt.Errorf("internal error: invalid Step: Number expected %v, was actually %v", i, stepModel.Number)) } - if stepModel.ID == "" { - stepModel.ID = fmt.Sprintf("%d", i) - } - stepModel.Number = i step, err := sf.newStep(stepModel, rc) if err != nil { diff --git a/act/runner/job_executor_test.go b/act/runner/job_executor_test.go index 918ece94..efe35e65 100644 --- a/act/runner/job_executor_test.go +++ b/act/runner/job_executor_test.go @@ -209,11 +209,14 @@ func TestJobExecutorNewJobExecutor(t *testing.T) { { name: "stepsWithPreAndPost", steps: []*model.Step{{ - ID: "1", + ID: "1", + Number: 0, }, { - ID: "2", + ID: "2", + Number: 1, }, { - ID: "3", + ID: "3", + Number: 2, }}, preSteps: []bool{true, false, true}, postSteps: []bool{false, true, true},