From 88ee53bfe3ed9aff80540ed2e108371b533293d9 Mon Sep 17 00:00:00 2001 From: Amit Mishra Date: Sat, 27 Jun 2026 12:46:08 +0530 Subject: [PATCH 1/3] fix: don't mutate shared matrix AST when resolving `ref:` rows resolveMatrixRefs mutated the *ast.Matrix passed to it in place. That Matrix is part of the cached, shared Task AST (the same *ast.For/*ast.Matrix is reused for every call to a task), so concurrent invocations of a task with a `for: matrix: ref:` loop - e.g. via parallel deps that call the same internal task with different vars - raced on the row.Value assignment. The result was silent cross-contamination: one caller's resolved ref value could leak into another caller's matrix expansion, non-deterministically. Matrix.DeepCopy() already existed but didn't actually deep-copy anything: deepcopy.OrderedMap only deep-copies a value if it implements the Copier interface, and *MatrixRow did not, so the "copy" still shared the same *MatrixRow pointers as the original. Fix: - Add MatrixRow.DeepCopy(), so Matrix.DeepCopy() produces independent rows. - resolveMatrixRefs now deep-copies the matrix before resolving refs into it, and returns the copy instead of mutating its input. Added a regression test that drives resolveMatrixRefs concurrently from many goroutines against the same shared matrix with different ref values, and asserts both that each caller gets its own value back and that the original shared matrix is never mutated. Reverting the fix reliably reproduces the exact cross-contamination described in the issue. Fixes #2890 --- taskfile/ast/matrix.go | 15 +++++++ variables.go | 23 +++++++--- variables_internal_test.go | 87 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 7 deletions(-) create mode 100644 variables_internal_test.go diff --git a/taskfile/ast/matrix.go b/taskfile/ast/matrix.go index aab0687d1b..8421b77e20 100644 --- a/taskfile/ast/matrix.go +++ b/taskfile/ast/matrix.go @@ -93,6 +93,21 @@ func (matrix *Matrix) DeepCopy() *Matrix { } } +// DeepCopy returns a copy of the MatrixRow. Without this, deepcopy.OrderedMap +// falls back to copying the *MatrixRow pointer as-is, so every "copy" of a +// Matrix would still share the same underlying rows - see #2890, where +// concurrent invocations of a task with a `ref:` matrix row raced on +// resolveMatrixRefs mutating that shared row. +func (row *MatrixRow) DeepCopy() *MatrixRow { + if row == nil { + return nil + } + return &MatrixRow{ + Ref: row.Ref, + Value: deepcopy.Slice(row.Value), + } +} + func (matrix *Matrix) UnmarshalYAML(node *yaml.Node) error { switch node.Kind { case yaml.MappingNode: diff --git a/variables.go b/variables.go index c7c6cc8493..7af5e3a43f 100644 --- a/variables.go +++ b/variables.go @@ -347,13 +347,14 @@ func itemsFromFor( var values []any // The list of values to loop over // Get the list from a matrix if f.Matrix.Len() != 0 { - if err := resolveMatrixRefs(f.Matrix, cache); err != nil { + resolvedMatrix, err := resolveMatrixRefs(f.Matrix, cache) + if err != nil { return nil, nil, errors.TaskfileInvalidError{ URI: location.Taskfile, Err: err, } } - return asAnySlice(product(f.Matrix)), nil, nil + return asAnySlice(product(resolvedMatrix)), nil, nil } // Get the list from the explicit for list if len(f.List) > 0 { @@ -425,22 +426,30 @@ func itemsFromFor( return values, keys, nil } -func resolveMatrixRefs(matrix *ast.Matrix, cache *templater.Cache) error { +// resolveMatrixRefs resolves any `ref:` rows in matrix and returns a new +// Matrix with those rows populated. It must not mutate the matrix passed in: +// that matrix is part of the shared, cached Task AST, and concurrent +// invocations of the same task (e.g. via parallel deps) call this with the +// same *ast.Matrix and would otherwise race on the row.Value assignment +// below, intermittently leaking a value resolved for one caller into another +// caller's expansion. See #2890. +func resolveMatrixRefs(matrix *ast.Matrix, cache *templater.Cache) (*ast.Matrix, error) { if matrix.Len() == 0 { - return nil + return matrix, nil } - for _, row := range matrix.All() { + resolved := matrix.DeepCopy() + for _, row := range resolved.All() { if row.Ref != "" { v := templater.ResolveRef(row.Ref, cache) switch value := v.(type) { case []any: row.Value = value default: - return fmt.Errorf("matrix reference %q must resolve to a list", row.Ref) + return nil, fmt.Errorf("matrix reference %q must resolve to a list", row.Ref) } } } - return nil + return resolved, nil } func resolveEnumRefs(requires *ast.Requires, cache *templater.Cache) error { diff --git a/variables_internal_test.go b/variables_internal_test.go new file mode 100644 index 0000000000..dd296b56a7 --- /dev/null +++ b/variables_internal_test.go @@ -0,0 +1,87 @@ +package task + +import ( + "sync" + "testing" + + "github.com/go-task/task/v3/internal/templater" + "github.com/go-task/task/v3/taskfile/ast" +) + +// TestResolveMatrixRefsDoesNotMutateSharedMatrix is a regression test for +// #2890. The *ast.Matrix passed to resolveMatrixRefs is part of the shared, +// cached Task AST, and concurrent invocations of the same task (e.g. via +// parallel deps) all call resolveMatrixRefs with the very same *ast.Matrix. +// If resolveMatrixRefs mutates that matrix in place, concurrent callers race +// on the mutation and can observe a value resolved for a different caller +// (cross-contamination), or trip the race detector. +// +// Run with `go test -race` to catch the race deterministically; it is not +// guaranteed to corrupt output on every unsynchronized run. +func TestResolveMatrixRefsDoesNotMutateSharedMatrix(t *testing.T) { + t.Parallel() + + sharedMatrix := ast.NewMatrix( + &ast.MatrixElement{Key: "ARCH", Value: &ast.MatrixRow{Ref: ".ARCH_VAR"}}, + ) + + const n = 200 + var wg sync.WaitGroup + results := make([]string, n) + errs := make([]error, n) + + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + + want := "amd64" + if i%2 != 0 { + want = "arm64" + } + + vars := ast.NewVars() + vars.Set("ARCH_VAR", ast.Var{Value: []any{want}}) + cache := &templater.Cache{Vars: vars} + + resolved, err := resolveMatrixRefs(sharedMatrix, cache) + if err != nil { + errs[i] = err + return + } + row, ok := resolved.Get("ARCH") + if !ok || len(row.Value) != 1 { + return + } + got, _ := row.Value[0].(string) + results[i] = got + }(i) + } + wg.Wait() + + for i, err := range errs { + if err != nil { + t.Fatalf("call %d: unexpected error: %v", i, err) + } + want := "amd64" + if i%2 != 0 { + want = "arm64" + } + if results[i] != want { + t.Errorf("call %d: got ARCH=%q, want %q (cross-contamination between concurrent callers)", i, results[i], want) + } + } + + // The original, shared matrix must remain unresolved: resolveMatrixRefs + // must operate on a copy, not the input. + origRow, ok := sharedMatrix.Get("ARCH") + if !ok { + t.Fatal("ARCH row missing from original matrix") + } + if origRow.Value != nil { + t.Errorf("shared input matrix was mutated: ARCH.Value = %v, want nil (Ref rows must be resolved into a copy)", origRow.Value) + } + if origRow.Ref != ".ARCH_VAR" { + t.Errorf("shared input matrix Ref was altered: got %q, want %q", origRow.Ref, ".ARCH_VAR") + } +} From 4641a5692fc6f3c81fe8fda7f8af0035dc8c77aa Mon Sep 17 00:00:00 2001 From: Amit Mishra Date: Sat, 27 Jun 2026 23:50:36 +0530 Subject: [PATCH 2/3] fix: address review feedback on matrix ref resolution - Skip the deep-copy in resolveMatrixRefs when the matrix has no ref: rows, avoiding unnecessary allocation on every invocation of a static matrix loop. - Check cache.Err() right after templater.ResolveRef, matching the existing pattern in resolveEnumRefs, so a real templating error isn't masked as "must resolve to a list". - In the regression test, record an explicit error instead of silently leaving a result empty when a goroutine's row lookup fails, so such failures aren't misreported as cross-contamination. --- variables.go | 13 +++++++++++++ variables_internal_test.go | 8 +++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/variables.go b/variables.go index 7af5e3a43f..733cfaf44f 100644 --- a/variables.go +++ b/variables.go @@ -437,10 +437,23 @@ func resolveMatrixRefs(matrix *ast.Matrix, cache *templater.Cache) (*ast.Matrix, if matrix.Len() == 0 { return matrix, nil } + hasRef := false + for _, row := range matrix.All() { + if row.Ref != "" { + hasRef = true + break + } + } + if !hasRef { + return matrix, nil + } resolved := matrix.DeepCopy() for _, row := range resolved.All() { if row.Ref != "" { v := templater.ResolveRef(row.Ref, cache) + if cache.Err() != nil { + return nil, cache.Err() + } switch value := v.(type) { case []any: row.Value = value diff --git a/variables_internal_test.go b/variables_internal_test.go index dd296b56a7..e8b13e6e22 100644 --- a/variables_internal_test.go +++ b/variables_internal_test.go @@ -1,6 +1,7 @@ package task import ( + "fmt" "sync" "testing" @@ -50,7 +51,12 @@ func TestResolveMatrixRefsDoesNotMutateSharedMatrix(t *testing.T) { return } row, ok := resolved.Get("ARCH") - if !ok || len(row.Value) != 1 { + if !ok { + errs[i] = fmt.Errorf("call %d: ARCH row missing from resolved matrix", i) + return + } + if len(row.Value) != 1 { + errs[i] = fmt.Errorf("call %d: ARCH.Value = %v, want a single-element slice", i, row.Value) return } got, _ := row.Value[0].(string) From 8c3892162db46e5f4a91d44140fb90b7a463e4c8 Mon Sep 17 00:00:00 2001 From: Valentin Maerten Date: Sun, 28 Jun 2026 21:54:11 +0200 Subject: [PATCH 3/3] test: simplify matrix ref regression test Rename variables_internal_test.go to variables_test.go and reduce the test to the deterministic invariant it actually guards: resolveMatrixRefs must resolve ref rows into a copy and leave the shared input matrix untouched. The previous 200-goroutine version only caught the race under -race, which this repo's CI does not run. The non-mutation assertion fails deterministically on the buggy in-place version, with or without -race. --- variables_internal_test.go | 93 -------------------------------------- variables_test.go | 45 ++++++++++++++++++ 2 files changed, 45 insertions(+), 93 deletions(-) delete mode 100644 variables_internal_test.go create mode 100644 variables_test.go diff --git a/variables_internal_test.go b/variables_internal_test.go deleted file mode 100644 index e8b13e6e22..0000000000 --- a/variables_internal_test.go +++ /dev/null @@ -1,93 +0,0 @@ -package task - -import ( - "fmt" - "sync" - "testing" - - "github.com/go-task/task/v3/internal/templater" - "github.com/go-task/task/v3/taskfile/ast" -) - -// TestResolveMatrixRefsDoesNotMutateSharedMatrix is a regression test for -// #2890. The *ast.Matrix passed to resolveMatrixRefs is part of the shared, -// cached Task AST, and concurrent invocations of the same task (e.g. via -// parallel deps) all call resolveMatrixRefs with the very same *ast.Matrix. -// If resolveMatrixRefs mutates that matrix in place, concurrent callers race -// on the mutation and can observe a value resolved for a different caller -// (cross-contamination), or trip the race detector. -// -// Run with `go test -race` to catch the race deterministically; it is not -// guaranteed to corrupt output on every unsynchronized run. -func TestResolveMatrixRefsDoesNotMutateSharedMatrix(t *testing.T) { - t.Parallel() - - sharedMatrix := ast.NewMatrix( - &ast.MatrixElement{Key: "ARCH", Value: &ast.MatrixRow{Ref: ".ARCH_VAR"}}, - ) - - const n = 200 - var wg sync.WaitGroup - results := make([]string, n) - errs := make([]error, n) - - for i := 0; i < n; i++ { - wg.Add(1) - go func(i int) { - defer wg.Done() - - want := "amd64" - if i%2 != 0 { - want = "arm64" - } - - vars := ast.NewVars() - vars.Set("ARCH_VAR", ast.Var{Value: []any{want}}) - cache := &templater.Cache{Vars: vars} - - resolved, err := resolveMatrixRefs(sharedMatrix, cache) - if err != nil { - errs[i] = err - return - } - row, ok := resolved.Get("ARCH") - if !ok { - errs[i] = fmt.Errorf("call %d: ARCH row missing from resolved matrix", i) - return - } - if len(row.Value) != 1 { - errs[i] = fmt.Errorf("call %d: ARCH.Value = %v, want a single-element slice", i, row.Value) - return - } - got, _ := row.Value[0].(string) - results[i] = got - }(i) - } - wg.Wait() - - for i, err := range errs { - if err != nil { - t.Fatalf("call %d: unexpected error: %v", i, err) - } - want := "amd64" - if i%2 != 0 { - want = "arm64" - } - if results[i] != want { - t.Errorf("call %d: got ARCH=%q, want %q (cross-contamination between concurrent callers)", i, results[i], want) - } - } - - // The original, shared matrix must remain unresolved: resolveMatrixRefs - // must operate on a copy, not the input. - origRow, ok := sharedMatrix.Get("ARCH") - if !ok { - t.Fatal("ARCH row missing from original matrix") - } - if origRow.Value != nil { - t.Errorf("shared input matrix was mutated: ARCH.Value = %v, want nil (Ref rows must be resolved into a copy)", origRow.Value) - } - if origRow.Ref != ".ARCH_VAR" { - t.Errorf("shared input matrix Ref was altered: got %q, want %q", origRow.Ref, ".ARCH_VAR") - } -} diff --git a/variables_test.go b/variables_test.go new file mode 100644 index 0000000000..60a923da07 --- /dev/null +++ b/variables_test.go @@ -0,0 +1,45 @@ +package task + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/go-task/task/v3/internal/templater" + "github.com/go-task/task/v3/taskfile/ast" +) + +// TestResolveMatrixRefsDoesNotMutateInput is a regression test for #2890. The +// *ast.Matrix passed to resolveMatrixRefs is part of the shared, cached Task +// AST: the same *ast.Matrix is reused on every invocation of a task. If +// resolveMatrixRefs resolved `ref:` rows in place, concurrent invocations of +// the same task (e.g. via parallel deps) would race on that mutation and leak +// a value resolved for one caller into another caller's expansion. +// +// The invariant that prevents this is that resolveMatrixRefs must resolve into +// a copy and leave its input untouched, which this test asserts deterministically. +func TestResolveMatrixRefsDoesNotMutateInput(t *testing.T) { + t.Parallel() + + matrix := ast.NewMatrix( + &ast.MatrixElement{Key: "ARCH", Value: &ast.MatrixRow{Ref: ".ARCH_VAR"}}, + ) + + vars := ast.NewVars() + vars.Set("ARCH_VAR", ast.Var{Value: []any{"amd64"}}) + cache := &templater.Cache{Vars: vars} + + resolved, err := resolveMatrixRefs(matrix, cache) + require.NoError(t, err) + + // The returned matrix has the ref resolved... + row, ok := resolved.Get("ARCH") + require.True(t, ok, "ARCH row missing from resolved matrix") + require.Equal(t, []any{"amd64"}, row.Value) + + // ...but the shared input matrix must be left untouched. + orig, ok := matrix.Get("ARCH") + require.True(t, ok, "ARCH row missing from input matrix") + require.Nil(t, orig.Value, "input matrix was mutated: Ref rows must be resolved into a copy") + require.Equal(t, ".ARCH_VAR", orig.Ref, "input matrix Ref was altered") +}