From d2bdef8bf72d4dcf60b58db199b2e019ef79a493 Mon Sep 17 00:00:00 2001 From: elijahr <153711+elijahr@users.noreply.github.com> Date: Thu, 11 Jun 2026 01:08:13 -0500 Subject: [PATCH 1/5] refactor(model): extract DecodeContainerNode shared helper Extract the scalar-vs-mapping decode switch from Job.Container() into an exported model.DecodeContainerNode(*yaml.Node) *ContainerSpec so the runner's per-RunContext container resolver can decode an evaluated node with byte-identical semantics. Job.Container() now delegates to it. Semantics are preserved exactly: empty scalar -> &ContainerSpec{Image:""} (not nil); decode error -> OnDecodeNodeError (fatal by default) -> nil; nil/zero node -> nil (host execution). --- pkg/model/workflow.go | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/pkg/model/workflow.go b/pkg/model/workflow.go index 8dd5f6634db..98ca1283d14 100644 --- a/pkg/model/workflow.go +++ b/pkg/model/workflow.go @@ -289,24 +289,41 @@ func (j *Job) Secrets() map[string]string { return val } -// Container details for the job -func (j *Job) Container() *ContainerSpec { +// DecodeContainerNode decodes a job-level `container:` yaml node into a +// ContainerSpec, replicating the exact scalar-vs-mapping semantics historically +// embedded in Job.Container(). It is exported so the runner's per-RunContext +// resolver can decode an *evaluated* node with byte-identical behavior. +// +// - Scalar node (including empty scalar ""): returns &ContainerSpec{Image: }; +// an empty scalar yields &ContainerSpec{Image: ""} (NOT nil). A decode *error* +// routes through OnDecodeNodeError (fatal by default) and returns nil. +// - Mapping node: full struct decode; decode error -> OnDecodeNodeError -> nil. +// - nil / zero (Kind == 0) node: returns nil (host execution). +func DecodeContainerNode(node *yaml.Node) *ContainerSpec { + if node == nil { + return nil + } var val *ContainerSpec - switch j.RawContainer.Kind { + switch node.Kind { case yaml.ScalarNode: val = new(ContainerSpec) - if !decodeNode(j.RawContainer, &val.Image) { + if !decodeNode(*node, &val.Image) { return nil } case yaml.MappingNode: val = new(ContainerSpec) - if !decodeNode(j.RawContainer, val) { + if !decodeNode(*node, val) { return nil } } return val } +// Container details for the job +func (j *Job) Container() *ContainerSpec { + return DecodeContainerNode(&j.RawContainer) +} + // Needs list for Job func (j *Job) Needs() []string { switch j.RawNeeds.Kind { From 74179f7b58a5d74dec7c4807ed7898f6e1d0af20 Mon Sep 17 00:00:00 2001 From: elijahr <153711+elijahr@users.noreply.github.com> Date: Thu, 11 Jun 2026 01:08:32 -0500 Subject: [PATCH 2/5] fix(runner): resolve container: ${{ matrix.container }} map indirection A job-level `container: ${{ matrix.container }}` where each matrix cell supplies a container MAP was stringified to the literal "Object" (pulling docker.io/library/Object), because Job.Container() decoded the unevaluated scalar and Interpolate's forceFormat=true coerced the map to "Object". Add a per-RunContext resolveJobContainer(ctx) that deep-copies the shared RawContainer yaml.Node, evaluates the COPY with the type-preserving EvaluateYamlNode (forceFormat=false), and decodes via the shared model.DecodeContainerNode helper. The result is memoized on a new resolvedJobContainer field. All five .Container() consumers (containerImage, options, GetBindsAndMounts, handleCredentials, mergeEnv) route through the resolver; GetBindsAndMounts gains a ctx parameter threaded from its three production callers. The shared *Job is NEVER mutated: cells run in parallel and share one *Job pointer, so in-place evaluation would leak across cells and race. Deep-copy + per-RunContext memo make resolution race-free by construction. When ExprEval is nil (a consumer reached before startJob sets it, or the bare-RC unit harness), the resolver falls back to a pure decode of the un-evaluated node, exactly reproducing the prior Job.Container() behavior. Tests: TestResolveJobContainer covers the distinct-image leak guard, sibling-field preservation, undefined-matrix host fallback (pinned to the observed empty-image-no-error branch), plain-string back-compat, nested-expr recursion, and nil-ExprEval pure-decode. TestResolveJobContainer_ParallelCells NoLeak proves race-freedom under -race. A Docker-guarded matrix-container integration fixture is added to the TestRunEvent table. --- pkg/runner/action.go | 2 +- pkg/runner/run_context.go | 85 +++++++++- pkg/runner/run_context_test.go | 158 +++++++++++++++++- pkg/runner/runner_test.go | 1 + pkg/runner/step.go | 3 +- pkg/runner/step_docker.go | 2 +- pkg/runner/testdata/matrix-container/push.yml | 15 ++ 7 files changed, 252 insertions(+), 14 deletions(-) create mode 100644 pkg/runner/testdata/matrix-container/push.yml diff --git a/pkg/runner/action.go b/pkg/runner/action.go index b46a4d03a75..5cf8621dde6 100644 --- a/pkg/runner/action.go +++ b/pkg/runner/action.go @@ -406,7 +406,7 @@ func newStepContainer(ctx context.Context, step step, image string, cmd []string envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_ARCH", container.RunnerArch(ctx))) envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_TEMP", "/tmp")) - binds, mounts := rc.GetBindsAndMounts() + binds, mounts := rc.GetBindsAndMounts(ctx) networkMode := fmt.Sprintf("container:%s", rc.jobContainerName()) var workdir string if rc.IsHostEnv(ctx) { diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go index 5d4277123ed..6480cf35b77 100644 --- a/pkg/runner/run_context.go +++ b/pkg/runner/run_context.go @@ -26,6 +26,7 @@ import ( "github.com/nektos/act/pkg/exprparser" "github.com/nektos/act/pkg/model" "github.com/opencontainers/selinux/go-selinux" + yaml "gopkg.in/yaml.v3" ) // RunContext contains info about current job @@ -53,6 +54,8 @@ type RunContext struct { caller *caller // job calling this RunContext (reusable workflows) Cancelled bool nodeToolFullPath string + + resolvedJobContainer *resolvedContainer // memoized per-cell resolved container spec } func (rc *RunContext) AddMask(mask string) { @@ -124,7 +127,7 @@ func getDockerDaemonSocketMountPath(daemonPath string) string { } // Returns the binds and mounts for the container, resolving paths as appropriate -func (rc *RunContext) GetBindsAndMounts() ([]string, map[string]string) { +func (rc *RunContext) GetBindsAndMounts(ctx context.Context) ([]string, map[string]string) { name := rc.jobContainerName() if rc.Config.ContainerDaemonSocket == "" { @@ -153,7 +156,7 @@ func (rc *RunContext) GetBindsAndMounts() ([]string, map[string]string) { } if job := rc.Run.Job(); job != nil { - if container := job.Container(); container != nil { + if container := rc.resolveJobContainer(ctx); container != nil { for _, v := range container.Volumes { if !strings.Contains(v, ":") || filepath.IsAbs(v) { // Bind anonymous volume or host file. @@ -283,7 +286,7 @@ func (rc *RunContext) startJobContainer() common.Executor { envList = append(envList, fmt.Sprintf("%s=%s", "LANG", "C.UTF-8")) // Use same locale as GitHub Actions ext := container.LinuxContainerEnvironmentExtensions{} - binds, mounts := rc.GetBindsAndMounts() + binds, mounts := rc.GetBindsAndMounts(ctx) // specify the network to which the container will connect when `docker create` stage. (like execute command line: docker create --network ) // if using service containers, will create a new network for the containers. @@ -731,10 +734,77 @@ func (rc *RunContext) Executor() (common.Executor, error) { }, nil } -func (rc *RunContext) containerImage(ctx context.Context) string { +// resolvedContainer wraps the per-cell resolved container spec so that "resolved +// to nil" (computed, no container) is distinguishable from "not yet resolved" +// (nil *resolvedContainer pointer). +type resolvedContainer struct { + spec *model.ContainerSpec +} + +// deepCopyYamlNode returns a recursively-cloned yaml.Node so that evaluation on +// the copy can never read or write storage shared with other matrix cells. +func deepCopyYamlNode(n *yaml.Node) *yaml.Node { + if n == nil { + return &yaml.Node{} + } + cp := *n // copy scalar fields (Kind, Tag, Value, Style, line info, ...) + cp.Content = nil + if len(n.Content) > 0 { + cp.Content = make([]*yaml.Node, len(n.Content)) + for i, child := range n.Content { + cp.Content[i] = deepCopyYamlNode(child) + } + } + if n.Alias != nil { + cp.Alias = deepCopyYamlNode(n.Alias) + } + return &cp +} + +// resolveJobContainer returns the per-RunContext container spec with the +// job-level `container:` field fully evaluated against this cell's matrix/env +// context. It deep-copies the shared RawContainer node so parallel matrix cells +// never mutate or race on the shared *Job. The result is memoized per RunContext. +// +// Returns nil when the job declares no container (host execution). +func (rc *RunContext) resolveJobContainer(ctx context.Context) *model.ContainerSpec { + if rc.resolvedJobContainer != nil { + return rc.resolvedJobContainer.spec + } + job := rc.Run.Job() - c := job.Container() + // Copy the shared node; never evaluate the shared RawContainer in place. + nodeCopy := deepCopyYamlNode(&job.RawContainer) + + // Empty / absent container -> host execution. Preserve existing nil semantics. + if nodeCopy.Kind == 0 { + rc.resolvedJobContainer = &resolvedContainer{spec: nil} + return nil + } + + // Type-preserving evaluation: a single `${{ matrix.container }}` scalar + // resolves to the actual map node; nested image/env expressions recurse. + // When there is no evaluator yet (e.g. a consumer reached before startJob sets + // ExprEval), fall back to a pure decode of the un-evaluated node, exactly + // reproducing the pre-resolver Job.Container() behavior. + if rc.ExprEval != nil { + if err := rc.ExprEval.EvaluateYamlNode(ctx, nodeCopy); err != nil { + common.Logger(ctx).Errorf("Error while evaluating container: %v", err) + rc.resolvedJobContainer = &resolvedContainer{spec: nil} + return nil + } + } + + // Same shared helper as Job.Container() -> identical decode semantics + // (empty-scalar -> Image:"", decode error -> OnDecodeNodeError). + spec := model.DecodeContainerNode(nodeCopy) + rc.resolvedJobContainer = &resolvedContainer{spec: spec} + return spec +} + +func (rc *RunContext) containerImage(ctx context.Context) string { + c := rc.resolveJobContainer(ctx) if c != nil { return rc.ExprEval.Interpolate(ctx, c.Image) } @@ -781,8 +851,7 @@ func (rc *RunContext) platformImage(ctx context.Context) string { } func (rc *RunContext) options(ctx context.Context) string { - job := rc.Run.Job() - c := job.Container() + c := rc.resolveJobContainer(ctx) if c != nil { return rc.ExprEval.Interpolate(ctx, c.Options) } @@ -1098,7 +1167,7 @@ func (rc *RunContext) handleCredentials(ctx context.Context) (string, string, er username := rc.Config.Secrets["DOCKER_USERNAME"] password := rc.Config.Secrets["DOCKER_PASSWORD"] - container := rc.Run.Job().Container() + container := rc.resolveJobContainer(ctx) if container == nil || container.Credentials == nil { return username, password, nil } diff --git a/pkg/runner/run_context_test.go b/pkg/runner/run_context_test.go index de725b037e6..6dd1d7e6501 100644 --- a/pkg/runner/run_context_test.go +++ b/pkg/runner/run_context_test.go @@ -1,6 +1,7 @@ package runner import ( + "bytes" "context" "fmt" "os" @@ -8,6 +9,7 @@ import ( "runtime" "sort" "strings" + "sync" "testing" "github.com/golang-jwt/jwt/v5" @@ -270,7 +272,7 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { config := testcase.rc.Config config.Workdir = testcase.name config.BindWorkdir = bindWorkDir - gotbind, gotmount := rctemplate.GetBindsAndMounts() + gotbind, gotmount := rctemplate.GetBindsAndMounts(context.Background()) // Name binds/mounts are either/or if config.BindWorkdir { @@ -322,7 +324,7 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { rc.Run.JobID = "job1" rc.Run.Workflow.Jobs = map[string]*model.Job{"job1": job} - gotbind, gotmount := rc.GetBindsAndMounts() + gotbind, gotmount := rc.GetBindsAndMounts(context.Background()) if len(testcase.wantbind) > 0 { assert.Contains(t, gotbind, testcase.wantbind) @@ -726,3 +728,155 @@ func TestSetRuntimeVariablesWithRunID(t *testing.T) { assert.True(t, ok, "scp claim exists") assert.Equal(t, "Actions.Results:45:45", scp, "contains expected scp claim") } + +func newContainerRC(ctx context.Context, t *testing.T, rawContainer interface{}, matrix map[string]interface{}) *RunContext { + t.Helper() + job := &model.Job{} + if rawContainer != nil { + if err := job.RawContainer.Encode(rawContainer); err != nil { + assert.NoError(t, err) + t.FailNow() + } + } + rc := &RunContext{ + Config: &Config{Workdir: "."}, + Run: &model.Run{ + JobID: "job1", + Workflow: &model.Workflow{ + Name: "test-workflow", + Jobs: map[string]*model.Job{"job1": job}, + }, + }, + Matrix: matrix, + } + rc.ExprEval = rc.NewExpressionEvaluator(ctx) + return rc +} + +func TestResolveJobContainer(t *testing.T) { + ctx := context.Background() + + t.Run("distinct images across cells (leak guard)", func(t *testing.T) { + job := &model.Job{} + assert.NoError(t, job.RawContainer.Encode("${{ matrix.container }}")) + wf := &model.Workflow{Name: "w", Jobs: map[string]*model.Job{"job1": job}} + mk := func(image string) *RunContext { + rc := &RunContext{ + Config: &Config{Workdir: "."}, + Run: &model.Run{JobID: "job1", Workflow: wf}, + Matrix: map[string]interface{}{"container": map[string]interface{}{"image": image}}, + } + rc.ExprEval = rc.NewExpressionEvaluator(ctx) + return rc + } + rcA, rcB := mk("img-a"), mk("img-b") + specA := rcA.resolveJobContainer(ctx) + specB := rcB.resolveJobContainer(ctx) + assert.NotNil(t, specA) + assert.NotNil(t, specB) + assert.Equal(t, "img-a", specA.Image) + assert.Equal(t, "img-b", specB.Image) + }) + + t.Run("sibling-field preservation", func(t *testing.T) { + rc := newContainerRC(ctx, t, "${{ matrix.container }}", map[string]interface{}{ + "container": map[string]interface{}{ + "image": "x", + "options": "--cpus 2", + "env": map[string]interface{}{"FOO": "bar"}, + "ports": []interface{}{"8080"}, + }, + }) + spec := rc.resolveJobContainer(ctx) + assert.NotNil(t, spec) + assert.Equal(t, "x", spec.Image) + assert.Equal(t, "--cpus 2", spec.Options) + assert.Equal(t, "bar", spec.Env["FOO"]) + assert.Equal(t, []string{"8080"}, spec.Ports) + }) + + t.Run("absent container -> host fallback (empty image, no Object leak)", func(t *testing.T) { + // matrix has no `container` key; the `${{ matrix.container }}` scalar is + // undefined. Observed-and-pinned current behavior (verified against + // expressionEvaluator.evaluateScalarYamlNode + EvaluateYamlNode, 2026-06-11): + // the expression evaluates to the null/undefined value, ret.Encode produces an + // empty scalar node, and DecodeContainerNode decodes that to + // &ContainerSpec{Image: ""} (NOT nil) -- identical to Job.Container() on an + // empty scalar. EvaluateYamlNode does NOT error, so the Errorf branch does NOT + // fire. The key host-fallback guarantee is that Image == "" (so containerImage + // falls through to runsOnImage) and the literal "Object" never appears. Pinning + // this exact branch makes a future evaluator change (e.g. one that errors or + // emits "Object") fail loudly. + var buf bytes.Buffer + prev := log.StandardLogger().Out + log.SetOutput(&buf) + defer log.SetOutput(prev) + + rc := newContainerRC(ctx, t, "${{ matrix.container }}", map[string]interface{}{"unused": "1"}) + spec := rc.resolveJobContainer(ctx) + assert.Equal(t, &model.ContainerSpec{Image: ""}, spec) + assert.NotContains(t, buf.String(), "Error while evaluating container") + }) + + t.Run("plain string unchanged (back-compat)", func(t *testing.T) { + rc := newContainerRC(ctx, t, "ghcr.io/foo/bar:tag", nil) + spec := rc.resolveJobContainer(ctx) + assert.NotNil(t, spec) + assert.Equal(t, "ghcr.io/foo/bar:tag", spec.Image) + }) + + t.Run("nested expr in map (recursive eval)", func(t *testing.T) { + rc := newContainerRC(ctx, t, map[string]interface{}{"image": "alpine:${{ matrix.tag }}"}, + map[string]interface{}{"tag": "3.19"}) + spec := rc.resolveJobContainer(ctx) + assert.NotNil(t, spec) + assert.Equal(t, "alpine:3.19", spec.Image) + }) + + t.Run("nil ExprEval -> pure decode, no panic (back-compat)", func(t *testing.T) { + // Some RunContexts (e.g. the GetBindsAndMounts volume-mount unit harness, and + // any consumer reached before startJob sets ExprEval) have a nil ExprEval. The + // pre-resolver job.Container() decode path required no evaluator, so the resolver + // MUST preserve that: with no evaluator, fall back to a pure decode of the + // un-evaluated node (identical to Job.Container()) instead of dereferencing nil. + job := &model.Job{} + assert.NoError(t, job.RawContainer.Encode(map[string][]string{"volumes": {"/volume"}})) + rc := &RunContext{ + Config: &Config{Workdir: "."}, + Run: &model.Run{ + JobID: "job1", + Workflow: &model.Workflow{Name: "w", Jobs: map[string]*model.Job{"job1": job}}, + }, + } + // ExprEval deliberately left nil. + spec := rc.resolveJobContainer(ctx) + assert.Equal(t, &model.ContainerSpec{Volumes: []string{"/volume"}}, spec) + }) +} + +func TestResolveJobContainer_ParallelCellsNoLeak(t *testing.T) { + job := &model.Job{} + assert.NoError(t, job.RawContainer.Encode("${{ matrix.container }}")) + wf := &model.Workflow{Name: "w", Jobs: map[string]*model.Job{"job1": job}} + + mk := func(image string) *RunContext { + rc := &RunContext{ + Config: &Config{Workdir: "."}, + Run: &model.Run{JobID: "job1", Workflow: wf}, + Matrix: map[string]interface{}{"container": map[string]interface{}{"image": image}}, + } + rc.ExprEval = rc.NewExpressionEvaluator(context.Background()) + return rc + } + rcA, rcB := mk("img-a"), mk("img-b") + + var wg sync.WaitGroup + var gotA, gotB string + wg.Add(2) + go func() { defer wg.Done(); gotA = rcA.resolveJobContainer(context.Background()).Image }() + go func() { defer wg.Done(); gotB = rcB.resolveJobContainer(context.Background()).Image }() + wg.Wait() + + assert.Equal(t, "img-a", gotA) + assert.Equal(t, "img-b", gotB) +} diff --git a/pkg/runner/runner_test.go b/pkg/runner/runner_test.go index 5c3959af2cd..7850f840bea 100644 --- a/pkg/runner/runner_test.go +++ b/pkg/runner/runner_test.go @@ -273,6 +273,7 @@ func TestRunEvent(t *testing.T) { {workdir, "job-container", "push", "", platforms, secrets}, {workdir, "job-container-non-root", "push", "", platforms, secrets}, {workdir, "job-container-invalid-credentials", "push", "failed to handle credentials: failed to interpolate container.credentials.password", platforms, secrets}, + {workdir, "matrix-container", "push", "", platforms, secrets}, {workdir, "container-hostname", "push", "", platforms, secrets}, {workdir, "remote-action-docker", "push", "", platforms, secrets}, {workdir, "remote-action-docker-new-cache", "push", "", platforms, secrets}, diff --git a/pkg/runner/step.go b/pkg/runner/step.go index 5ee0dbcf050..01f4331dc6a 100644 --- a/pkg/runner/step.go +++ b/pkg/runner/step.go @@ -272,9 +272,8 @@ func setupEnv(ctx context.Context, step step) error { func mergeEnv(ctx context.Context, step step) { env := step.getEnv() rc := step.getRunContext() - job := rc.Run.Job() - c := job.Container() + c := rc.resolveJobContainer(ctx) if c != nil { mergeIntoMap(step, env, rc.GetEnv(), c.Env) } else { diff --git a/pkg/runner/step_docker.go b/pkg/runner/step_docker.go index cc28a3b53c5..1d8bd1dff34 100644 --- a/pkg/runner/step_docker.go +++ b/pkg/runner/step_docker.go @@ -112,7 +112,7 @@ func (sd *stepDocker) newStepContainer(ctx context.Context, image string, cmd [] envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_ARCH", container.RunnerArch(ctx))) envList = append(envList, fmt.Sprintf("%s=%s", "RUNNER_TEMP", "/tmp")) - binds, mounts := rc.GetBindsAndMounts() + binds, mounts := rc.GetBindsAndMounts(ctx) stepContainer := ContainerNewContainer(&container.NewContainerInput{ Cmd: cmd, Entrypoint: entrypoint, diff --git a/pkg/runner/testdata/matrix-container/push.yml b/pkg/runner/testdata/matrix-container/push.yml new file mode 100644 index 00000000000..809746ab073 --- /dev/null +++ b/pkg/runner/testdata/matrix-container/push.yml @@ -0,0 +1,15 @@ +name: matrix-container +on: push +jobs: + test: + strategy: + matrix: + include: + - container: + image: ghcr.io/catthehacker/ubuntu:act-latest + - container: + image: ghcr.io/catthehacker/ubuntu:act-latest + runs-on: ubuntu-latest + container: ${{ matrix.container }} + steps: + - run: echo "resolved container image OK" From 472580bab112f3c9a1d1dfb8ee769243f54d6ba5 Mon Sep 17 00:00:00 2001 From: elijahr <153711+elijahr@users.noreply.github.com> Date: Thu, 11 Jun 2026 04:14:05 -0500 Subject: [PATCH 3/5] fix(runner): add nil guards to job-container resolver Addresses review feedback: defensive nil checks for rc.Run/job and nil-safe deepCopyYamlNode. --- pkg/runner/run_context.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go index 6480cf35b77..985f008f594 100644 --- a/pkg/runner/run_context.go +++ b/pkg/runner/run_context.go @@ -745,7 +745,7 @@ type resolvedContainer struct { // the copy can never read or write storage shared with other matrix cells. func deepCopyYamlNode(n *yaml.Node) *yaml.Node { if n == nil { - return &yaml.Node{} + return nil } cp := *n // copy scalar fields (Kind, Tag, Value, Style, line info, ...) cp.Content = nil @@ -772,13 +772,21 @@ func (rc *RunContext) resolveJobContainer(ctx context.Context) *model.ContainerS return rc.resolvedJobContainer.spec } + if rc.Run == nil { + rc.resolvedJobContainer = &resolvedContainer{spec: nil} + return nil + } job := rc.Run.Job() + if job == nil { + rc.resolvedJobContainer = &resolvedContainer{spec: nil} + return nil + } // Copy the shared node; never evaluate the shared RawContainer in place. nodeCopy := deepCopyYamlNode(&job.RawContainer) // Empty / absent container -> host execution. Preserve existing nil semantics. - if nodeCopy.Kind == 0 { + if nodeCopy == nil || nodeCopy.Kind == 0 { rc.resolvedJobContainer = &resolvedContainer{spec: nil} return nil } From 2bfd8a225c80b76a1e5138bb50b7fb9c94065df4 Mon Sep 17 00:00:00 2001 From: elijahr <153711+elijahr@users.noreply.github.com> Date: Thu, 11 Jun 2026 04:24:31 -0500 Subject: [PATCH 4/5] fix(runner): avoid memoizing unevaluated container; drop redundant interpolation Addresses review feedback: memoize the resolved container only when ExprEval is set (avoids caching an un-evaluated spec), and remove the redundant, nil-unsafe Interpolate calls in containerImage/options since resolveJobContainer already fully evaluates the node. --- pkg/runner/run_context.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go index 985f008f594..a91c269c9ac 100644 --- a/pkg/runner/run_context.go +++ b/pkg/runner/run_context.go @@ -807,14 +807,16 @@ func (rc *RunContext) resolveJobContainer(ctx context.Context) *model.ContainerS // Same shared helper as Job.Container() -> identical decode semantics // (empty-scalar -> Image:"", decode error -> OnDecodeNodeError). spec := model.DecodeContainerNode(nodeCopy) - rc.resolvedJobContainer = &resolvedContainer{spec: spec} + if rc.ExprEval != nil { + rc.resolvedJobContainer = &resolvedContainer{spec: spec} + } return spec } func (rc *RunContext) containerImage(ctx context.Context) string { c := rc.resolveJobContainer(ctx) if c != nil { - return rc.ExprEval.Interpolate(ctx, c.Image) + return c.Image } return "" @@ -861,7 +863,7 @@ func (rc *RunContext) platformImage(ctx context.Context) string { func (rc *RunContext) options(ctx context.Context) string { c := rc.resolveJobContainer(ctx) if c != nil { - return rc.ExprEval.Interpolate(ctx, c.Options) + return c.Options } return rc.Config.ContainerOptions From 609ed3acd2600b858aa2784cd238ef34db46e052 Mon Sep 17 00:00:00 2001 From: elijahr <153711+elijahr@users.noreply.github.com> Date: Thu, 11 Jun 2026 04:29:11 -0500 Subject: [PATCH 5/5] style: trim comments to match codebase --- pkg/model/workflow.go | 11 +---------- pkg/runner/run_context.go | 31 ++++++++----------------------- pkg/runner/run_context_test.go | 19 ++----------------- 3 files changed, 11 insertions(+), 50 deletions(-) diff --git a/pkg/model/workflow.go b/pkg/model/workflow.go index 98ca1283d14..ac880167ab1 100644 --- a/pkg/model/workflow.go +++ b/pkg/model/workflow.go @@ -289,16 +289,7 @@ func (j *Job) Secrets() map[string]string { return val } -// DecodeContainerNode decodes a job-level `container:` yaml node into a -// ContainerSpec, replicating the exact scalar-vs-mapping semantics historically -// embedded in Job.Container(). It is exported so the runner's per-RunContext -// resolver can decode an *evaluated* node with byte-identical behavior. -// -// - Scalar node (including empty scalar ""): returns &ContainerSpec{Image: }; -// an empty scalar yields &ContainerSpec{Image: ""} (NOT nil). A decode *error* -// routes through OnDecodeNodeError (fatal by default) and returns nil. -// - Mapping node: full struct decode; decode error -> OnDecodeNodeError -> nil. -// - nil / zero (Kind == 0) node: returns nil (host execution). +// DecodeContainerNode decodes a job-level container yaml node into a ContainerSpec func DecodeContainerNode(node *yaml.Node) *ContainerSpec { if node == nil { return nil diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go index a91c269c9ac..938f6d126b6 100644 --- a/pkg/runner/run_context.go +++ b/pkg/runner/run_context.go @@ -55,7 +55,7 @@ type RunContext struct { Cancelled bool nodeToolFullPath string - resolvedJobContainer *resolvedContainer // memoized per-cell resolved container spec + resolvedJobContainer *resolvedContainer // memoized per-cell resolved container } func (rc *RunContext) AddMask(mask string) { @@ -734,20 +734,16 @@ func (rc *RunContext) Executor() (common.Executor, error) { }, nil } -// resolvedContainer wraps the per-cell resolved container spec so that "resolved -// to nil" (computed, no container) is distinguishable from "not yet resolved" -// (nil *resolvedContainer pointer). +// resolvedContainer distinguishes a memoized nil spec from a not-yet-resolved container. type resolvedContainer struct { spec *model.ContainerSpec } -// deepCopyYamlNode returns a recursively-cloned yaml.Node so that evaluation on -// the copy can never read or write storage shared with other matrix cells. func deepCopyYamlNode(n *yaml.Node) *yaml.Node { if n == nil { return nil } - cp := *n // copy scalar fields (Kind, Tag, Value, Style, line info, ...) + cp := *n cp.Content = nil if len(n.Content) > 0 { cp.Content = make([]*yaml.Node, len(n.Content)) @@ -761,12 +757,9 @@ func deepCopyYamlNode(n *yaml.Node) *yaml.Node { return &cp } -// resolveJobContainer returns the per-RunContext container spec with the -// job-level `container:` field fully evaluated against this cell's matrix/env -// context. It deep-copies the shared RawContainer node so parallel matrix cells -// never mutate or race on the shared *Job. The result is memoized per RunContext. -// -// Returns nil when the job declares no container (host execution). +// resolveJobContainer evaluates the job container against this cell's context, +// deep-copying RawContainer so parallel matrix cells never evaluate the shared node +// in place. The result is memoized; nil means host execution. func (rc *RunContext) resolveJobContainer(ctx context.Context) *model.ContainerSpec { if rc.resolvedJobContainer != nil { return rc.resolvedJobContainer.spec @@ -782,20 +775,14 @@ func (rc *RunContext) resolveJobContainer(ctx context.Context) *model.ContainerS return nil } - // Copy the shared node; never evaluate the shared RawContainer in place. nodeCopy := deepCopyYamlNode(&job.RawContainer) - - // Empty / absent container -> host execution. Preserve existing nil semantics. if nodeCopy == nil || nodeCopy.Kind == 0 { rc.resolvedJobContainer = &resolvedContainer{spec: nil} return nil } - // Type-preserving evaluation: a single `${{ matrix.container }}` scalar - // resolves to the actual map node; nested image/env expressions recurse. - // When there is no evaluator yet (e.g. a consumer reached before startJob sets - // ExprEval), fall back to a pure decode of the un-evaluated node, exactly - // reproducing the pre-resolver Job.Container() behavior. + // A nil ExprEval (consumer reached before startJob) falls back to a pure + // decode of the un-evaluated node, matching Job.Container(). if rc.ExprEval != nil { if err := rc.ExprEval.EvaluateYamlNode(ctx, nodeCopy); err != nil { common.Logger(ctx).Errorf("Error while evaluating container: %v", err) @@ -804,8 +791,6 @@ func (rc *RunContext) resolveJobContainer(ctx context.Context) *model.ContainerS } } - // Same shared helper as Job.Container() -> identical decode semantics - // (empty-scalar -> Image:"", decode error -> OnDecodeNodeError). spec := model.DecodeContainerNode(nodeCopy) if rc.ExprEval != nil { rc.resolvedJobContainer = &resolvedContainer{spec: spec} diff --git a/pkg/runner/run_context_test.go b/pkg/runner/run_context_test.go index 6dd1d7e6501..b67c2c458f8 100644 --- a/pkg/runner/run_context_test.go +++ b/pkg/runner/run_context_test.go @@ -796,17 +796,8 @@ func TestResolveJobContainer(t *testing.T) { }) t.Run("absent container -> host fallback (empty image, no Object leak)", func(t *testing.T) { - // matrix has no `container` key; the `${{ matrix.container }}` scalar is - // undefined. Observed-and-pinned current behavior (verified against - // expressionEvaluator.evaluateScalarYamlNode + EvaluateYamlNode, 2026-06-11): - // the expression evaluates to the null/undefined value, ret.Encode produces an - // empty scalar node, and DecodeContainerNode decodes that to - // &ContainerSpec{Image: ""} (NOT nil) -- identical to Job.Container() on an - // empty scalar. EvaluateYamlNode does NOT error, so the Errorf branch does NOT - // fire. The key host-fallback guarantee is that Image == "" (so containerImage - // falls through to runsOnImage) and the literal "Object" never appears. Pinning - // this exact branch makes a future evaluator change (e.g. one that errors or - // emits "Object") fail loudly. + // An undefined `${{ matrix.container }}` resolves to an empty scalar, which + // decodes to &ContainerSpec{Image: ""} (NOT nil) without erroring. var buf bytes.Buffer prev := log.StandardLogger().Out log.SetOutput(&buf) @@ -834,11 +825,6 @@ func TestResolveJobContainer(t *testing.T) { }) t.Run("nil ExprEval -> pure decode, no panic (back-compat)", func(t *testing.T) { - // Some RunContexts (e.g. the GetBindsAndMounts volume-mount unit harness, and - // any consumer reached before startJob sets ExprEval) have a nil ExprEval. The - // pre-resolver job.Container() decode path required no evaluator, so the resolver - // MUST preserve that: with no evaluator, fall back to a pure decode of the - // un-evaluated node (identical to Job.Container()) instead of dereferencing nil. job := &model.Job{} assert.NoError(t, job.RawContainer.Encode(map[string][]string{"volumes": {"/volume"}})) rc := &RunContext{ @@ -848,7 +834,6 @@ func TestResolveJobContainer(t *testing.T) { Workflow: &model.Workflow{Name: "w", Jobs: map[string]*model.Job{"job1": job}}, }, } - // ExprEval deliberately left nil. spec := rc.resolveJobContainer(ctx) assert.Equal(t, &model.ContainerSpec{Volumes: []string{"/volume"}}, spec) })