From d63d70c0805522d5340092f09bcaf4eb2f12f6b4 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Fri, 15 Aug 2025 19:19:54 +0000 Subject: [PATCH] chore: prevent 'false positive' data race detection with Job.If [skip cascade] (#864) Initialization of the default value of the `If` field is functionally safe, but triggers the data race detector. ``` ================== WARNING: DATA RACE Read at 0x00c00037cf78 by goroutine 10: code.forgejo.org/forgejo/runner/v9/act/model.(*Workflow).GetJob() /.../forgejo-runner/act/model/workflow.go:766 +0x2ae code.forgejo.org/forgejo/runner/v9/act/model.(*Run).Job() /.../forgejo-runner/act/model/planner.go:50 +0xab code.forgejo.org/forgejo/runner/v9/act/runner.setJobResult() /.../forgejo-runner/act/runner/job_executor.go:168 +0x7c code.forgejo.org/forgejo/runner/v9/act/runner.TestSetJobResultConcurrency.func3() /.../forgejo-runner/act/runner/job_executor_test.go:410 +0xf8 Previous write at 0x00c00037cf78 by goroutine 9: code.forgejo.org/forgejo/runner/v9/act/model.(*Workflow).GetJob() /.../forgejo-runner/act/model/workflow.go:767 +0x2ce code.forgejo.org/forgejo/runner/v9/act/model.(*Run).Job() /.../forgejo-runner/act/model/planner.go:50 +0xab code.forgejo.org/forgejo/runner/v9/act/runner.setJobResult() /.../forgejo-runner/act/runner/job_executor.go:168 +0x7c code.forgejo.org/forgejo/runner/v9/act/runner.TestSetJobResultConcurrency.func2() /.../forgejo-runner/act/runner/job_executor_test.go:405 +0xfb Goroutine 10 (running) created at: code.forgejo.org/forgejo/runner/v9/act/runner.TestSetJobResultConcurrency() /.../forgejo-runner/act/runner/job_executor_test.go:408 +0xbac testing.tRunner() /home/mfenniak/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/src/testing/testing.go:1792 +0x225 testing.(*T).Run.gowrap1() /home/mfenniak/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/src/testing/testing.go:1851 +0x44 Goroutine 9 (running) created at: code.forgejo.org/forgejo/runner/v9/act/runner.TestSetJobResultConcurrency() /.../forgejo-runner/act/runner/job_executor_test.go:403 +0xa84 testing.tRunner() /home/mfenniak/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/src/testing/testing.go:1792 +0x225 testing.(*T).Run.gowrap1() /home/mfenniak/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.6.linux-amd64/src/testing/testing.go:1851 +0x44 ================== ``` - other - [PR](https://code.forgejo.org/forgejo/runner/pulls/864): chore: prevent 'false positive' data race detection with Job.If [skip cascade] Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/864 Reviewed-by: earl-warren Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- act/model/workflow.go | 12 ++++++++---- act/runner/run_context.go | 6 +++--- act/runner/runner.go | 2 +- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/act/model/workflow.go b/act/model/workflow.go index 3462d288..5ca423c3 100644 --- a/act/model/workflow.go +++ b/act/model/workflow.go @@ -207,7 +207,7 @@ type Job struct { RawNeeds yaml.Node `yaml:"needs"` RawRunsOn yaml.Node `yaml:"runs-on"` Env yaml.Node `yaml:"env"` - If yaml.Node `yaml:"if"` + RawIf yaml.Node `yaml:"if"` Steps []*Step `yaml:"steps"` TimeoutMinutes string `yaml:"timeout-minutes"` Services map[string]*ContainerSpec `yaml:"services"` @@ -362,6 +362,13 @@ func (j *Job) RunsOn() []string { } } +func (j *Job) IfClause() string { + if j.RawIf.Value == "" { + return "success()" + } + return j.RawIf.Value +} + func nodeAsStringSlice(node yaml.Node) []string { switch node.Kind { case yaml.ScalarNode: @@ -761,9 +768,6 @@ func (w *Workflow) GetJob(jobID string) *Job { if j.Name == "" { j.Name = id } - if j.If.Value == "" { - j.If.Value = "success()" - } return j } } diff --git a/act/runner/run_context.go b/act/runner/run_context.go index 5ed2bb35..85398e39 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -1022,11 +1022,11 @@ func (rc *RunContext) options(ctx context.Context) string { func (rc *RunContext) isEnabled(ctx context.Context) (bool, error) { job := rc.Run.Job() l := common.Logger(ctx) - runJob, runJobErr := EvalBool(ctx, rc.ExprEval, job.If.Value, exprparser.DefaultStatusCheckSuccess) + runJob, runJobErr := EvalBool(ctx, rc.ExprEval, job.IfClause(), exprparser.DefaultStatusCheckSuccess) jobType, jobTypeErr := job.Type() if runJobErr != nil { - return false, fmt.Errorf(" \u274C Error in if-expression: \"if: %s\" (%s)", job.If.Value, runJobErr) + return false, fmt.Errorf(" \u274C Error in if-expression: \"if: %s\" (%s)", job.IfClause(), runJobErr) } if jobType == model.JobTypeInvalid { @@ -1035,7 +1035,7 @@ func (rc *RunContext) isEnabled(ctx context.Context) (bool, error) { if !runJob { rc.result("skipped") - l.WithField("jobResult", "skipped").Infof("Skipping job '%s' due to '%s'", job.Name, job.If.Value) + l.WithField("jobResult", "skipped").Infof("Skipping job '%s' due to '%s'", job.Name, job.IfClause()) return false, nil } diff --git a/act/runner/runner.go b/act/runner/runner.go index 7fd5139e..29b4cc7a 100644 --- a/act/runner/runner.go +++ b/act/runner/runner.go @@ -154,7 +154,7 @@ func (runner *runnerImpl) NewPlanExecutor(plan *model.Plan) common.Executor { log.Debugf("Job.RawNeeds: %v", job.RawNeeds) log.Debugf("Job.RawRunsOn: %v", job.RawRunsOn) log.Debugf("Job.Env: %v", job.Env) - log.Debugf("Job.If: %v", job.If) + log.Debugf("Job.If: %v", job.IfClause()) for step := range job.Steps { if nil != job.Steps[step] { log.Debugf("Job.Steps: %v", job.Steps[step].String())