fix: don't mutate shared matrix AST when resolving ref: rows (#2894)

Co-authored-by: Valentin Maerten <maerten.valentin@gmail.com>

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:

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,43 @@ 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
 	}
+	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
 			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 {

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")
+}