fix(actions): dynamic matrix expansion correctness fixes (#38690)

Follow-up to https://github.com/go-gitea/gitea/pull/36564 (dynamic
matrix) and https://github.com/go-gitea/gitea/pull/36357 (max-parallel),
fixing issues found reviewing the two features together.

- **A placeholder could stall its run forever.** Its payload keeps the
raw matrix but loses its `needs`, so `ParseJob` re-expanded it instead
of reading it back — fatal for `include: ${{ fromJson(needs.*.outputs.*)
}}`.
- **An `if:` reading `matrix.*` skipped the whole job**, with or without
the `${{ }}`. It now reduces to the needs gate, except under
`always()`/`failure()`/`cancelled()`, and each combination is decided on
its own values once the matrix expands.
- **Dependents could be skipped before the combinations ran**, since
inserted siblings are absent from the resolver's job set. The pass now
stops after an insert and defers to the re-emit it schedules.
- **Expansion failures stranded the placeholder.** A retryable one is
returned so the queue retries it; a malformed payload fails the job
instead of requeueing forever.
- **Rerun could rewind a pass-through row** into a raw placeholder
keeping its old terminal status, which nothing expands. Now gated on the
anchor itself.

Plus: `max-parallel` distinguishes an unevaluated `${{ }}` (debug) from
a non-numeric literal (warn — it silently drops the cap).

Co-authored-by: Zettat123 <zettat123@gmail.com>
Co-authored-by: silverwind <me@silverwind.io>

modules/actions/jobparser/jobparser.go

@@ -32,11 +32,72 @@ func rawMatrixReadsNeeds(node *yaml.Node) bool {
 	return slices.ContainsFunc(node.Content, rawMatrixReadsNeeds)
 }
 
+// ParseRawSingleWorkflow decodes a SingleWorkflow payload into the workflow and its single job
+// without expanding `strategy.matrix`.
+//
+// A deferred-matrix placeholder's payload still carries the raw, unevaluated matrix, which Parse
+// would try to expand: depending on the matrix's shape that yields several workflows (a static
+// vector crossed with the unevaluated expression) or an error (an `include`/`exclude` that is still
+// a scalar), neither of which describes the one job the payload stands for.
+func ParseRawSingleWorkflow(payload []byte) (*SingleWorkflow, *Job, error) {
+	swf := &SingleWorkflow{}
+	if err := yaml.Unmarshal(payload, swf); err != nil {
+		return nil, nil, fmt.Errorf("unmarshal single workflow: %w", err)
+	}
+	id, job := swf.Job()
+	if job == nil {
+		return nil, nil, errors.New("payload contains no job")
+	}
+	if job.Name == "" {
+		job.Name = id // Parse defaults it the same way, and callers use it as the job's display name
+	}
+	return swf, job, nil
+}
+
 // expressionReadsNeeds reports whether value holds a ${{ }} expression reading the needs context.
 // Every other context (github, vars, inputs, ...) is already available while planning, so deferring
 // those too would replace their combinations with one placeholder and change the commit status
 // contexts the run publishes, which a repository's required checks are configured against.
 func expressionReadsNeeds(value string) bool {
+	return expressionReadsContext(value, "needs")
+}
+
+// ExpressionReadsMatrix reports whether a job's `if:` reads the matrix context.
+// A deferred-matrix placeholder has no combination yet, so such an expression cannot be decided.
+func ExpressionReadsMatrix(ifValue string) bool {
+	return expressionReadsContext(asIfExpression(ifValue), "matrix")
+}
+
+// ExpressionIgnoresNeedResults reports whether a job's `if:` calls always(), failure() or cancelled(),
+// the status functions that run a job whatever its needs did rather than under the implicit success().
+// Keep in sync with act's exprparser, which owns the same list for the evaluation itself.
+func ExpressionIgnoresNeedResults(ifValue string) bool {
+	return expressionsMatch(asIfExpression(ifValue), func(node actionlint.ExprNode) bool {
+		call, ok := node.(*actionlint.FuncCallNode)
+		return ok && slices.Contains([]string{"always", "failure", "cancelled"}, strings.ToLower(call.Callee))
+	})
+}
+
+// asIfExpression wraps an `if:` that omits the `${{ }}`, which GitHub evaluates as one expression anyway.
+// `if:` is the only field with that exception: every other value is interpolated, so a bare matrix or
+// `runs-on` is a literal there and must not be parsed as an expression.
+func asIfExpression(ifValue string) string {
+	if ifValue == "" || strings.Contains(ifValue, "${{") {
+		return ifValue
+	}
+	return "${{ " + ifValue + " }}"
+}
+
+// expressionReadsContext reports whether value holds a ${{ }} expression reading the named context.
+func expressionReadsContext(value, contextName string) bool {
+	return expressionsMatch(value, func(node actionlint.ExprNode) bool {
+		variable, ok := node.(*actionlint.VariableNode)
+		return ok && strings.EqualFold(variable.Name, contextName)
+	})
+}
+
+// expressionsMatch reports whether any ${{ }} expression in value holds a node the predicate accepts.
+func expressionsMatch(value string, match func(node actionlint.ExprNode) bool) bool {
 	for rest := value; ; {
 		_, after, found := strings.Cut(rest, "${{")
 		if !found {
@@ -48,13 +109,13 @@ func expressionReadsNeeds(value string) bool {
 		if err != nil {
 			return true // unparseable here, let the expansion report it against the real values
 		}
-		readsNeeds := false
+		matched := false
 		actionlint.VisitExprNode(expr, func(node, _ actionlint.ExprNode, entering bool) {
-			if variable, ok := node.(*actionlint.VariableNode); entering && ok && strings.EqualFold(variable.Name, "needs") {
-				readsNeeds = true
+			if entering && match(node) {
+				matched = true
 			}
 		})
-		if readsNeeds {
+		if matched {
 			return true
 		}
 	}
@@ -124,7 +185,7 @@ func Parse(content []byte, options ...ParseOption) ([]*SingleWorkflow, error) {
 			}
 		}
 		for _, combo := range combos {
-			swf := workflow.cloneHeader()
+			swf := workflow.CloneHeader()
 			if err := swf.SetJob(id, combo); err != nil {
 				return nil, fmt.Errorf("SetJob: %w", err)
 			}
@@ -134,8 +195,8 @@ func Parse(content []byte, options ...ParseOption) ([]*SingleWorkflow, error) {
 	return ret, nil
 }
 
-// cloneHeader returns a copy of w with its workflow-global fields but no jobs.
-func (w *SingleWorkflow) cloneHeader() *SingleWorkflow {
+// CloneHeader returns a copy of w with its workflow-global fields but no jobs.
+func (w *SingleWorkflow) CloneHeader() *SingleWorkflow {
 	return &SingleWorkflow{
 		Name:           w.Name,
 		RawOn:          w.RawOn,

modules/actions/jobparser/jobparser_test.go

@@ -236,23 +236,168 @@ func TestExpandMatrixWithNeeds(t *testing.T) {
 	})
 }
 
+// evaluateJobIf builds a one-job workflow around the given `matrix:` value and `if:`, and decides it.
+func evaluateJobIf(t *testing.T, matrixYAML, ifExpr string, deferred bool) (bool, error) {
+	t.Helper()
+	var strategy Strategy
+	require.NoError(t, yaml.Unmarshal(fmt.Appendf(nil, "matrix:\n  %s\n", matrixYAML), &strategy))
+	job := &Job{Name: "build", Strategy: strategy}
+	require.NoError(t, job.If.Encode(ifExpr))
+	return EvaluateJobIfExpression("build", job, map[string]any{}, map[string]*JobResult{"build": {}}, nil, nil, deferred)
+}
+
 func TestRejectsUnevaluatedMatrixFilters(t *testing.T) {
 	// act dereferences include/exclude entries as mappings without checking, so an unevaluated
-	// expression panics there. Every entry point into act's matrix expansion must reject it: Parse
-	// sees one in a workflow file and in a deferred placeholder's payload, and the emitter reads a
-	// placeholder's `if:` while its matrix is still raw.
+	// expression panics there. Every entry point into act's matrix expansion must reject it. The
+	// expression here reads `vars`, which is available while planning, so the job is not deferred and
+	// nothing will ever resolve the filter: the error is the right answer at both entry points.
+	// A deferred placeholder is the other case, covered by TestEvaluateJobIfExpressionLeavesRawMatrixUnavailable.
 	for _, filter := range []string{"include", "exclude"} {
 		t.Run(filter, func(t *testing.T) {
 			_, err := Parse(fmt.Appendf(nil,
 				"name: t\non: push\njobs:\n  build:\n    runs-on: ubuntu-latest\n    strategy:\n      matrix:\n        os: [a]\n        %s: ${{ fromJson(vars.MATRIX) }}\n    steps: [{run: echo}]\n", filter))
 			require.ErrorContains(t, err, "must be a list of mappings")
 
-			var strategy Strategy
-			require.NoError(t, yaml.Unmarshal(fmt.Appendf(nil, "matrix:\n  os: [a]\n  %s: ${{ fromJson(vars.MATRIX) }}\n", filter), &strategy))
-			job := &Job{Name: "build", Strategy: strategy}
-			require.NoError(t, job.If.Encode("${{ true }}"))
-			_, err = EvaluateJobIfExpression("build", job, map[string]any{}, map[string]*JobResult{"build": {}}, nil, nil)
+			_, err = evaluateJobIf(t, fmt.Sprintf("os: [a]\n  %s: ${{ fromJson(vars.MATRIX) }}", filter), "${{ true }}", false)
 			require.ErrorContains(t, err, "must be a list of mappings")
 		})
 	}
 }
+
+func TestParseRawSingleWorkflowRoundTripsDeferredPlaceholder(t *testing.T) {
+	// The server persists a placeholder the way insertRunJob does: erase the needs, then marshal.
+	// Reading it back must yield that one job again. Parse cannot do it: it only keeps a matrix raw
+	// while the job still declares needs, so on the stored payload it falls through to expanding the
+	// raw matrix instead - which either fails or splits the placeholder into several workflows, and
+	// in both cases leaves the job unexpandable for good.
+	const workflow = `
+on: push
+jobs:
+  setup:
+    steps: [{run: echo}]
+  build:
+    needs: setup
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        %s
+    steps: [{run: echo}]
+`
+	for _, tt := range []struct {
+		name        string
+		matrix      string
+		parseCount  int    // what Parse makes of the stored payload
+		parseErrHas string // ... or the error it fails with
+	}{
+		// The canonical GitHub dynamic-matrix idiom. act dereferences include entries as mappings, so
+		// validateMatrixFilters rejects the still-scalar expression outright.
+		{name: "include expression", matrix: "include: ${{ fromJson(needs.setup.outputs.m) }}", parseErrHas: "must be a list of mappings"},
+		// A static vector crossed with the unevaluated expression: one workflow per static value.
+		{name: "static vector and expression", matrix: "os: [a, b]\n        version: ${{ fromJson(needs.setup.outputs.m) }}", parseCount: 2},
+		// The single-key case the feature shipped with happens to survive Parse, so it must keep working.
+		{name: "single expression vector", matrix: "version: ${{ fromJson(needs.setup.outputs.m) }}", parseCount: 1},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			planned, err := Parse(fmt.Appendf(nil, workflow, tt.matrix))
+			require.NoError(t, err)
+
+			var payload []byte
+			for _, w := range planned {
+				id, job := w.Job()
+				if id != "build" {
+					continue
+				}
+				require.True(t, HasDeferredMatrix(job), "build must be planned as a placeholder")
+				require.NoError(t, w.SetJob(id, job.EraseNeeds()))
+				payload, err = w.Marshal()
+				require.NoError(t, err)
+			}
+			require.NotEmpty(t, payload, "no placeholder was planned for build")
+
+			// The stored payload keeps the raw matrix, but no longer the needs that made Parse defer it.
+			_, job, err := ParseRawSingleWorkflow(payload)
+			require.NoError(t, err)
+			assert.Equal(t, "build", job.Name)
+			// The needs are gone, which is exactly why Parse no longer defers this payload.
+			assert.Empty(t, job.Needs())
+			assert.False(t, HasDeferredMatrix(job))
+
+			// Guard the reason ParseRawSingleWorkflow exists, so a future Parse change cannot quietly
+			// make the placeholder re-expandable again without this being noticed.
+			reparsed, err := Parse(payload)
+			if tt.parseErrHas != "" {
+				require.ErrorContains(t, err, tt.parseErrHas)
+			} else {
+				require.NoError(t, err)
+				assert.Len(t, reparsed, tt.parseCount)
+			}
+		})
+	}
+}
+
+func TestEvaluateJobIfExpressionLeavesRawMatrixUnavailable(t *testing.T) {
+	// A placeholder's `if:` is read before its matrix can be resolved. `matrix.*` has to be absent
+	// there: binding it to the expression's own source text would decide the job against a value no
+	// combination ever has, and an include/exclude that is still a scalar cannot be read at all.
+	t.Run("include expression is not read", func(t *testing.T) {
+		run, err := evaluateJobIf(t, "include: ${{ fromJson(needs.setup.outputs.m) }}", "${{ true }}", true)
+		require.NoError(t, err)
+		assert.True(t, run)
+	})
+
+	t.Run("matrix context is null, not the raw expression", func(t *testing.T) {
+		const matrix = "version: ${{ fromJson(needs.setup.outputs.m) }}"
+		run, err := evaluateJobIf(t, matrix, "${{ matrix.version == null }}", true)
+		require.NoError(t, err)
+		assert.True(t, run)
+
+		run, err = evaluateJobIf(t, matrix, "${{ matrix.version == '${{ fromJson(needs.setup.outputs.m) }}' }}", true)
+		require.NoError(t, err)
+		assert.False(t, run)
+	})
+
+	t.Run("an expanded job still reads its combination", func(t *testing.T) {
+		run, err := evaluateJobIf(t, "version: [1]", "${{ matrix.version == 1 }}", false)
+		require.NoError(t, err)
+		assert.True(t, run)
+	})
+}
+
+func TestExpressionReadsMatrix(t *testing.T) {
+	// Erring toward true only postpones the `if:` to the pass that has the combination, which decides it correctly anyway.
+	for value, want := range map[string]bool{
+		"":                                  false,
+		"true":                              false, // a bare literal is an expression too, it just reads nothing
+		"${{ always() }}":                   false,
+		"${{ needs.setup.result == 'ok' }}": false,
+		"${{ vars.MATRIX }}":                false, // a name that merely looks like the context
+		"${{ matrix.os }}":                  true,
+		"${{ MATRIX.os }}":                  true, // contexts are case-insensitive
+		"${{ always() && matrix.os == 1 }}": true,
+		"${{ contains(matrix.tags, 'a') }}": true,
+		"${{ toJSON(matrix) }}":             true, // the whole context, not a property of it
+		"${{ vars.A }}${{ matrix.os }}":     true, // only the second of two expressions reads it
+		"${{ matrix.os == }}":               true, // unparseable, postpone rather than decide it here
+		// An `if:` may omit the `${{ }}`, and is evaluated as one expression either way.
+		"matrix.os == 'a'":           true,
+		"needs.setup.result == 'ok'": false,
+	} {
+		assert.Equal(t, want, ExpressionReadsMatrix(value), "value %q", value)
+	}
+}
+
+func TestExpressionIgnoresNeedResults(t *testing.T) {
+	for value, want := range map[string]bool{
+		"":                             false,
+		"${{ matrix.os == 'a' }}":      false,
+		"${{ success() }}":             false, // the implicit gate, so the fallback already matches it
+		"${{ always() }}":              true,
+		"${{ ALWAYS() && matrix.os }}": true, // function names are case-insensitive
+		"${{ failure() }}":             true,
+		"${{ cancelled() }}":           true,
+		"always() && matrix.os == 'a'": true, // the brace-less form of the same gate
+		"${{ vars.always }}":           false,
+	} {
+		assert.Equal(t, want, ExpressionIgnoresNeedResults(value), "value %q", value)
+	}
+}

modules/actions/jobparser/model.go

@@ -497,7 +497,8 @@ func ParseRawOn(rawOn *yaml.Node) ([]*Event, error) {
 	}
 }
 
-func EvaluateJobIfExpression(jobID string, job *Job, gitCtx map[string]any, results map[string]*JobResult, vars map[string]string, inputs map[string]any) (bool, error) {
+// EvaluateJobIfExpression evaluates a job's `if:`.
+func EvaluateJobIfExpression(jobID string, job *Job, gitCtx map[string]any, results map[string]*JobResult, vars map[string]string, inputs map[string]any, matrixDeferred bool) (bool, error) {
 	actJob := &model.Job{
 		Strategy: &model.Strategy{
 			FailFastString:    job.Strategy.FailFastString,
@@ -509,13 +510,20 @@ func EvaluateJobIfExpression(jobID string, job *Job, gitCtx map[string]any, resu
 	// otherwise `matrix.*` references in `if:` evaluate to null.
 	// GetMatrixes always returns at least one element (an empty map for a job without a matrix),
 	// so only a non-empty combination should populate `matrix.*`, leaving it nil otherwise.
+	//
+	// A deferred-matrix placeholder is the exception: its combinations do not exist yet, and reading the
+	// raw matrix here would either fail outright (an `include` that is still a scalar expression) or bind
+	// `matrix.*` to the expression's own source text. Leaving it nil is safe: the caller checks
+	// ExpressionReadsMatrix first, so an `if:` that reads `matrix.*` is deferred to the post-expansion pass.
 	var matrix map[string]any
-	matrixes, err := matrixesOf(actJob)
-	if err != nil {
-		return false, err
-	}
-	if len(matrixes) > 0 && len(matrixes[0]) > 0 {
-		matrix = matrixes[0]
+	if !matrixDeferred {
+		matrixes, err := matrixesOf(actJob)
+		if err != nil {
+			return false, err
+		}
+		if len(matrixes) > 0 && len(matrixes[0]) > 0 {
+			matrix = matrixes[0]
+		}
 	}
 	evaluator := NewExpressionEvaluator(NewInterpeter(jobID, actJob, matrix, toGitContext(gitCtx), results, vars, inputs))
 	expr, err := rewriteSubExpression(job.If.Value, false)

modules/actions/jobparser/model_test.go

@@ -502,7 +502,7 @@ jobs:
 			got := make(map[string]bool, len(swfs))
 			for _, swf := range swfs {
 				id, job := swf.Job()
-				shouldRun, err := EvaluateJobIfExpression(id, job, map[string]any{}, map[string]*JobResult{id: {}}, nil, nil)
+				shouldRun, err := EvaluateJobIfExpression(id, job, map[string]any{}, map[string]*JobResult{id: {}}, nil, nil, false)
 				require.NoError(t, err)
 				got[job.Name] = shouldRun
 			}
@@ -562,7 +562,7 @@ jobs:
 				"job1": {Result: kase.needResult},
 				"job2": {Needs: []string{"job1"}},
 			}
-			got, err := EvaluateJobIfExpression("job2", job2, map[string]any{}, results, nil, nil)
+			got, err := EvaluateJobIfExpression("job2", job2, map[string]any{}, results, nil, nil, false)
 			require.NoError(t, err)
 			assert.Equal(t, kase.expected, got)
 		})