diff --git a/models/actions/run_job.go b/models/actions/run_job.go index b4cf1caa94..8090431059 100644 --- a/models/actions/run_job.go +++ b/models/actions/run_job.go @@ -192,6 +192,16 @@ func (job *ActionRunJob) LoadAttributes(ctx context.Context) error { // ParseJob parses the job structure from the ActionRunJob.WorkflowPayload func (job *ActionRunJob) ParseJob() (*jobparser.Job, error) { + if job.IsMatrixDeferred { + // The needs were erased before the placeholder was persisted, so jobparser.Parse no longer + // recognises the raw matrix it still carries and would re-expand it: see ParseRawSingleWorkflow. + _, workflowJob, err := jobparser.ParseRawSingleWorkflow(job.WorkflowPayload) + if err != nil { + return nil, fmt.Errorf("job %d deferred matrix placeholder: unable to parse: %w", job.ID, err) + } + return workflowJob, nil + } + // job.WorkflowPayload is a SingleWorkflow created from an ActionRun's workflow, which exactly contains this job's YAML definition. // Ideally it shouldn't be called "Workflow", it is just a job with global workflow fields + trigger parsedWorkflows, err := jobparser.Parse(job.WorkflowPayload) diff --git a/models/actions/run_job_test.go b/models/actions/run_job_test.go index 4437b5906d..5bc91e4cf5 100644 --- a/models/actions/run_job_test.go +++ b/models/actions/run_job_test.go @@ -4,6 +4,7 @@ package actions import ( + "fmt" "testing" "gitea.dev/models/db" @@ -197,3 +198,43 @@ func TestCancelJobs_NestedBlockedReusableCaller(t *testing.T) { gotRun := unittest.AssertExistsAndLoadBean(t, &ActionRun{ID: run.ID}) assert.Equal(t, StatusCancelled, gotRun.Status, "run must aggregate to Cancelled, not stay Blocked") } + +func TestParseJobDeferredMatrixPlaceholder(t *testing.T) { + // A placeholder is persisted with the raw matrix and without its needs, so routing its payload + // through jobparser.Parse re-expands that matrix. The job emitter reads `if:` (and so ParseJob) + // before it can expand the placeholder, and it only logs a parse failure: getting this wrong + // leaves the job Blocked on every pass, the run never finishes and its concurrency group is + // never released. + payload := func(matrix string) []byte { + return fmt.Appendf(nil, `name: test +on: push +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + %s + steps: + - run: echo +`, matrix) + } + + // The shape Parse happens to survive, so only the name tells the two paths apart. What Parse makes + // of every other shape is asserted in TestParseRawSingleWorkflowRoundTripsDeferredPlaceholder. + t.Run("a placeholder is read back, not re-expanded", func(t *testing.T) { + job := &ActionRunJob{ID: 1, JobID: "build", IsMatrixDeferred: true, WorkflowPayload: payload("version: ${{ fromJson(needs.setup.outputs.m) }}")} + parsed, err := job.ParseJob() + require.NoError(t, err) + require.NotNil(t, parsed) + assert.Equal(t, "build", parsed.Name) + }) + + t.Run("an expanded job still goes through the full parse", func(t *testing.T) { + job := &ActionRunJob{ID: 1, JobID: "build", WorkflowPayload: payload("version: [1]")} + parsed, err := job.ParseJob() + require.NoError(t, err) + require.NotNil(t, parsed) + // Parse bakes the combination into the name, ParseRawSingleWorkflow would not. + assert.Equal(t, "build (1)", parsed.Name) + }) +} diff --git a/modules/actions/jobparser/jobparser.go b/modules/actions/jobparser/jobparser.go index 5981d27d0c..d6ce23daa4 100644 --- a/modules/actions/jobparser/jobparser.go +++ b/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, diff --git a/modules/actions/jobparser/jobparser_test.go b/modules/actions/jobparser/jobparser_test.go index 96749f9c28..00fc4bdc22 100644 --- a/modules/actions/jobparser/jobparser_test.go +++ b/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) + } +} diff --git a/modules/actions/jobparser/model.go b/modules/actions/jobparser/model.go index 6b423c038e..4406bb7715 100644 --- a/modules/actions/jobparser/model.go +++ b/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) diff --git a/modules/actions/jobparser/model_test.go b/modules/actions/jobparser/model_test.go index 23d7489fed..3ceb3f196f 100644 --- a/modules/actions/jobparser/model_test.go +++ b/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) }) diff --git a/services/actions/helper.go b/services/actions/helper.go index 5c85823fac..3c55169579 100644 --- a/services/actions/helper.go +++ b/services/actions/helper.go @@ -11,6 +11,7 @@ import ( "gitea.dev/modules/actions/jobparser" "gitea.dev/modules/json" api "gitea.dev/modules/structs" + "gitea.dev/modules/util" ) func getWorkflowDispatchInputsFromRun(run *actions_model.ActionRun) (map[string]any, error) { @@ -42,7 +43,7 @@ func getInputsForJob(ctx context.Context, run *actions_model.ActionRun, job *act } var p api.WorkflowCallPayload if err := json.Unmarshal([]byte(caller.CallPayload), &p); err != nil { - return nil, fmt.Errorf("decode caller %d payload: %w", caller.ID, err) + return nil, util.NewInvalidArgumentErrorf("decode caller %d payload: %v", caller.ID, err) } if p.Inputs == nil { return map[string]any{}, nil @@ -60,6 +61,12 @@ func evaluateJobIf(ctx context.Context, run *actions_model.ActionRun, attempt *a if len(parsedJob.If.Value) == 0 { return allNeedsSucceed, nil } + // A deferred-matrix placeholder has no combination yet, so an `if:` reading `matrix.*` can only be + // decided by the emitter's post-expansion pass, against each combination's own values. + // always()/failure()/cancelled() opt out of the needs gate this falls back to. + if job.IsMatrixDeferred && jobparser.ExpressionReadsMatrix(parsedJob.If.Value) { + return allNeedsSucceed || jobparser.ExpressionIgnoresNeedResults(parsedJob.If.Value), nil + } jobResults, err := findJobNeedsAndFillJobResults(ctx, job) if err != nil { return false, err @@ -77,7 +84,7 @@ func evaluateJobIf(ctx context.Context, run *actions_model.ActionRun, attempt *a return false, err } gitCtx := GenerateGiteaContext(ctx, run, attempt, job) - return jobparser.EvaluateJobIfExpression(job.JobID, parsedJob, gitCtx, jobResults, vars, inputs) + return jobparser.EvaluateJobIfExpression(job.JobID, parsedJob, gitCtx, jobResults, vars, inputs, job.IsMatrixDeferred) } func findJobNeedsAndFillJobResults(ctx context.Context, job *actions_model.ActionRunJob) (map[string]*jobparser.JobResult, error) { diff --git a/services/actions/helper_test.go b/services/actions/helper_test.go new file mode 100644 index 0000000000..b6172e44a2 --- /dev/null +++ b/services/actions/helper_test.go @@ -0,0 +1,60 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package actions + +import ( + "fmt" + "testing" + + actions_model "gitea.dev/models/actions" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestEvaluateJobIfDefersMatrixExpression(t *testing.T) { + // A placeholder's `if:` reading `matrix.*` can only be decided per combination once the matrix is expanded. + + emptyRun := &actions_model.ActionRun{} // if the gate is removed, the emptyRun will cause an error + deferredJob := func(ifExpr string) *actions_model.ActionRunJob { + return &actions_model.ActionRunJob{ + ID: 1, JobID: "build", Needs: []string{"setup"}, IsMatrixDeferred: true, + WorkflowPayload: fmt.Appendf(nil, `name: test +on: push +jobs: + build: + runs-on: ubuntu-latest + if: %s + strategy: + matrix: + value: ${{ fromJson(needs.setup.outputs.m) }} + steps: + - run: echo +`, ifExpr), + } + } + + for _, tt := range []struct { + ifExpr string + // wantNeedsFailed is what the `if:` decides when the needs did not all succeed. + wantNeedsFailed bool + }{ + // These hold only for a real combination, so none can be decided before the matrix expands, + // and the fallback is the needs gate: a job whose needs did not succeed is still skipped. + {ifExpr: "${{ matrix.value == 1 }}"}, + {ifExpr: "matrix.value == 1"}, // an `if:` may omit the `${{ }}` + // always() asks to run whatever the needs did, so it must not fall back to their gate. + {ifExpr: "${{ always() && matrix.value == 1 }}", wantNeedsFailed: true}, + } { + t.Run(tt.ifExpr, func(t *testing.T) { + got, err := evaluateJobIf(t.Context(), emptyRun, nil, deferredJob(tt.ifExpr), nil, true) + require.NoError(t, err) + assert.True(t, got, "must reach the expansion that gates each combination on its own values") + + got, err = evaluateJobIf(t.Context(), emptyRun, nil, deferredJob(tt.ifExpr), nil, false) + require.NoError(t, err) + assert.Equal(t, tt.wantNeedsFailed, got) + }) + } +} diff --git a/services/actions/job_emitter.go b/services/actions/job_emitter.go index 6e5f30a915..577bc66d89 100644 --- a/services/actions/job_emitter.go +++ b/services/actions/job_emitter.go @@ -361,6 +361,8 @@ type jobStatusResolver struct { // matrixChanged is set when matrix expansion inserted siblings or failed a placeholder, both of // which need a follow-up pass to resolve the dependents. matrixChanged bool + // matrixInserted is set when matrix expansion inserted sibling rows, which Resolve stops on. + matrixInserted bool // matrixUpdatedJobs holds jobs whose status matrix expansion persisted itself, so they are // notified like the ones the caller updates from the resolved status map. matrixUpdatedJobs []*actions_model.ActionRunJob @@ -418,6 +420,14 @@ func (r *jobStatusResolver) Resolve(ctx context.Context) (map[int64]actions_mode ret[k] = v r.statuses[k] = v } + if r.matrixInserted { + // Matrix expansion inserted sibling rows this round. They are not in statuses/needs, so + // another round would resolve a dependent of the expanded job against the placeholder's + // own combination alone: if that combination was just skipped by its `if:`, the dependent + // sees all its needs done and gets skipped before any sibling has even started. Stop here + // and let the re-emit, which reloads the full job set, resolve them. + return ret, nil + } } return ret, nil } @@ -472,8 +482,8 @@ func (r *jobStatusResolver) resolve(ctx context.Context) (map[int64]actions_mode // Decide whether the job runs at all before expanding a deferred matrix: a job whose needs // failed or were skipped has to be skipped too, not failed for a matrix those needs never - // produced the outputs for. A job-level `if:` cannot read `matrix.*`, so it does not need - // the combination, unlike the concurrency expression evaluated below. + // produced the outputs for. An `if:` that reads `matrix.*` cannot be decided this early, so + // evaluateJobIf reduces it to that needs gate and the pass below decides it per combination. shouldStartJob, err := evaluateJobIf(ctx, actionRunJob.Run, nil, actionRunJob, r.vars, allSucceed) if err != nil { // TODO: surface deterministic expression errors to users by failing the job with a message. @@ -489,8 +499,10 @@ func (r *jobStatusResolver) resolve(ctx context.Context) (map[int64]actions_mode wasDeferred := actionRunJob.IsMatrixDeferred siblings, err := expandDeferredMatrix(ctx, actionRunJob, r.vars) if err != nil { - // Aborting the pass is required: the placeholder is already claimed as the first - // combination, so committing here would drop the remaining ones for good. + // Aborting the pass is required: once the placeholder is claimed as the first combination, + // committing here would drop the remaining ones for good. Before the claim it is what gets + // the pass retried by the job-emitter queue, since a run whose needs are all done has + // nothing left to trigger another pass on its own. return nil, fmt.Errorf("expand matrix of job %d: %w", id, err) } if actionRunJob.Status != actions_model.StatusBlocked { @@ -503,10 +515,12 @@ func (r *jobStatusResolver) resolve(ctx context.Context) (map[int64]actions_mode if actionRunJob.IsMatrixDeferred { continue // could not be expanded yet, it stays blocked and is retried on the next pass } - r.matrixChanged = r.matrixChanged || len(siblings) > 0 + if len(siblings) > 0 { + r.matrixChanged, r.matrixInserted = true, true + } if wasDeferred { - // The `if:` above was decided against the raw matrix, so this row still has to be gated - // by its own combination like the siblings are on the next pass. + // This row is now the first combination, and the `if:` can be evaluated. + // Gate it on its own combination here, as the siblings will be on the next pass. shouldStartJob, err := evaluateJobIf(ctx, actionRunJob.Run, nil, actionRunJob, r.vars, allSucceed) if err != nil { log.Error("evaluateJobIf failed after matrix expansion, job will stay blocked: job: %d, err: %v", id, err) diff --git a/services/actions/job_emitter_test.go b/services/actions/job_emitter_test.go index f9930b6485..df4d74a12a 100644 --- a/services/actions/job_emitter_test.go +++ b/services/actions/job_emitter_test.go @@ -589,3 +589,39 @@ func Test_maxParallelReusableCallerLifecycle(t *testing.T) { assert.Equal(t, len(callers), statusCounts(callers)[actions_model.StatusSuccess]) } + +// Test_jobStatusResolverStopsAfterMatrixInsert covers the invariant that keeps a dynamic matrix's +// dependents honest: a round resolved after an insert would judge them against a job set that is +// missing the siblings. See Resolve for why that is wrong. +func Test_jobStatusResolverStopsAfterMatrixInsert(t *testing.T) { + ctx := t.Context() + + // build (2) stands for the expanded anchor: it reaches a terminal status this round, which is + // what would let report (3) resolve in the next one. + newChain := func() actions_model.ActionJobList { + return actions_model.ActionJobList{ + {ID: 1, JobID: "generate", Status: actions_model.StatusFailure, WorkflowPayload: minimalWorkflowPayload("generate")}, + {ID: 2, JobID: "build", Status: actions_model.StatusBlocked, Needs: []string{"generate"}, WorkflowPayload: minimalWorkflowPayload("build")}, + {ID: 3, JobID: "report", Status: actions_model.StatusBlocked, Needs: []string{"build"}, WorkflowPayload: minimalWorkflowPayload("report")}, + } + } + + t.Run("without an insert the whole chain resolves in one pass", func(t *testing.T) { + got, err := newJobStatusResolver(newChain(), nil).Resolve(ctx) + require.NoError(t, err) + assert.Equal(t, map[int64]actions_model.Status{ + 2: actions_model.StatusSkipped, + 3: actions_model.StatusSkipped, + }, got) + }) + + t.Run("an insert stops the pass before the dependents are resolved", func(t *testing.T) { + r := newJobStatusResolver(newChain(), nil) + r.matrixInserted = true // as resolve() sets it once expansion has inserted siblings + + got, err := r.Resolve(ctx) + require.NoError(t, err) + assert.Equal(t, map[int64]actions_model.Status{2: actions_model.StatusSkipped}, got, + "report must wait for the re-emit, which sees the sibling combinations too") + }) +} diff --git a/services/actions/matrix.go b/services/actions/matrix.go index 86225a8b39..30e9bc9f6e 100644 --- a/services/actions/matrix.go +++ b/services/actions/matrix.go @@ -17,7 +17,6 @@ import ( "gitea.dev/modules/timeutil" "gitea.dev/modules/util" - "go.yaml.in/yaml/v4" "xorm.io/builder" ) @@ -28,16 +27,19 @@ import ( // // It runs inside the caller's transaction (job_emitter's resolver) and must not open a nested // db.WithTx, which would reuse the ambient session and roll the whole emitter pass back on error. -// The three outcomes are reported through the job itself: +// The three outcomes are: // - expanded: IsMatrixDeferred is cleared and the job stays StatusBlocked. // - the workflow's fault (a matrix that cannot resolve, or one too large): a terminal status is // persisted here, reported by the job leaving StatusBlocked. IsMatrixDeferred stays set, marking // the payload as still unexpanded for a later rerun to re-derive. -// - not now: the job is left deferred and blocked for the next emitter pass to retry. This covers -// a transient failure before anything was written, and losing the claim to a concurrent pass. +// - not now: losing the claim to a concurrent pass leaves the job deferred and blocked, for the +// pass that won the claim to carry through. // -// A returned error is reserved for a failure after the placeholder was claimed and must roll the -// caller's transaction back: committing it would drop the remaining combinations for good. +// Every other failure is returned, which aborts the emitter pass and lets the job-emitter queue retry +// it. Swallowing one would strand the placeholder: nothing re-emits a run whose needs have all already +// finished, so there would be no next pass to retry in. After the placeholder has been claimed a +// returned error additionally has to roll the caller's transaction back, because committing it would +// drop the remaining combinations for good. func expandDeferredMatrix(ctx context.Context, job *actions_model.ActionRunJob, vars map[string]string) ([]*actions_model.ActionRunJob, error) { if !job.IsMatrixDeferred { return nil, nil @@ -47,7 +49,7 @@ func expandDeferredMatrix(ctx context.Context, job *actions_model.ActionRunJob, // reusable caller (a job may be both) would drop: its branch only handles waiting and skipped. // The commit status is unaffected by the surviving flag: suppression only applies while the job // is not done. - failTerminal := func(cause error) ([]*actions_model.ActionRunJob, error) { + failTerminal := func(cause error) error { log.Warn("Matrix expansion failed for job %d (JobID: %s): %v", job.ID, job.JobID, cause) prevStatus, prevStopped := job.Status, job.Stopped job.Status = actions_model.StatusFailure @@ -58,52 +60,45 @@ func expandDeferredMatrix(ctx context.Context, job *actions_model.ActionRunJob, builder.Eq{"is_matrix_deferred": true, "status": actions_model.StatusBlocked}, "status", "stopped") if err != nil { - return nil, fmt.Errorf("fail deferred matrix job %d: %w", job.ID, err) + return fmt.Errorf("fail deferred matrix job %d: %w", job.ID, err) } if affected != 1 { // A concurrent pass already advanced the row. Restore the in-memory state so this pass // does not report a failure that was never persisted. job.Status, job.Stopped = prevStatus, prevStopped } - return nil, nil - } - - // retryLater leaves the placeholder untouched. It is only used before anything is written, so a - // transient failure neither fails the job nor aborts the emitter pass for the whole run. - retryLater := func(cause error) ([]*actions_model.ActionRunJob, error) { - log.Error("Matrix expansion of job %d (JobID: %s) postponed to the next pass: %v", job.ID, job.JobID, cause) - return nil, nil + return nil } // The resolver only calls this once every need is done, as it does for job concurrency. results, err := findJobNeedsAndFillJobResults(ctx, job) if err != nil { - return retryLater(fmt.Errorf("find needs: %w", err)) + return nil, fmt.Errorf("find needs: %w", err) } if err := job.LoadAttributes(ctx); err != nil { - return retryLater(fmt.Errorf("load attributes: %w", err)) + return nil, fmt.Errorf("load attributes: %w", err) } // The payload still carries the raw, unevaluated matrix: planning only erases the needs. - var baseSWF jobparser.SingleWorkflow - if err := yaml.Unmarshal(job.WorkflowPayload, &baseSWF); err != nil { - return failTerminal(fmt.Errorf("unmarshal payload: %w", err)) - } - _, parsedJob := baseSWF.Job() - if parsedJob == nil { - return failTerminal(errors.New("payload contains no job")) + baseSWF, parsedJob, err := jobparser.ParseRawSingleWorkflow(job.WorkflowPayload) + if err != nil { + return nil, failTerminal(fmt.Errorf("parse payload: %w", err)) } // `strategy` may reference the inputs context as well as needs, so resolve it like `if:` does. inputs, err := getInputsForJob(ctx, job.Run, job) if err != nil { - return retryLater(fmt.Errorf("get inputs: %w", err)) + if errors.Is(err, util.ErrInvalidArgument) { + // A malformed payload never becomes readable, so retrying would requeue the run forever. + return nil, failTerminal(fmt.Errorf("get inputs: %w", err)) + } + return nil, fmt.Errorf("get inputs: %w", err) } existingJobs, err := actions_model.CountRunJobsByRunAndAttemptID(ctx, job.RunID, job.RunAttemptID) if err != nil { - return retryLater(fmt.Errorf("count jobs of attempt %d: %w", job.RunAttemptID, err)) + return nil, fmt.Errorf("count jobs of attempt %d: %w", job.RunAttemptID, err) } // The placeholder is reused as the first combination, so the attempt only grows by len-1. maxCombinations := int(actions_model.MaxJobNumPerRun - existingJobs + 1) @@ -111,12 +106,12 @@ func expandDeferredMatrix(ctx context.Context, job *actions_model.ActionRunJob, giteaCtx := GenerateGiteaContext(ctx, job.Run, nil, job) expandedJobs, err := jobparser.ExpandMatrixWithNeeds(job.JobID, parsedJob, giteaCtx.ToGitHubContext(), results, vars, inputs, maxCombinations) if err != nil { - return failTerminal(fmt.Errorf("expand matrix: %w", err)) + return nil, failTerminal(fmt.Errorf("expand matrix: %w", err)) } // Combinations differ only in what the matrix feeds: the name, the payload, and a // runs-on/continue-on-error that may interpolate matrix.*. applyCombo := func(dst *actions_model.ActionRunJob, combo *jobparser.Job) error { - swf := baseSWF + swf := baseSWF.CloneHeader() if err := swf.SetJob(job.JobID, combo.EraseNeeds()); err != nil { return fmt.Errorf("set expanded job: %w", err) } @@ -142,7 +137,7 @@ func expandDeferredMatrix(ctx context.Context, job *actions_model.ActionRunJob, // Only the placeholder keeps the raw payload, which is what identifies it as the group's anchor. sibling.DeferredMatrixPayload = nil if err := applyCombo(&sibling, combo); err != nil { - return failTerminal(err) + return nil, failTerminal(err) } siblings = append(siblings, &sibling) } @@ -154,13 +149,13 @@ func expandDeferredMatrix(ctx context.Context, job *actions_model.ActionRunJob, if job.ParentJobID > 0 { parent, err := actions_model.GetRunJobByRunAndID(ctx, job.RunID, job.ParentJobID) if err != nil { - return retryLater(fmt.Errorf("load parent of job %d: %w", job.ID, err)) + return nil, fmt.Errorf("load parent of job %d: %w", job.ID, err) } parentAttemptJobID = parent.AttemptJobID } priorCombos, err := actions_model.GetPriorAttemptMatrixCombos(ctx, job.RunID, job.RunAttemptID, parentAttemptJobID, job.JobID) if err != nil { - return retryLater(fmt.Errorf("lookup prior attempt combos of job %d: %w", job.ID, err)) + return nil, fmt.Errorf("lookup prior attempt combos of job %d: %w", job.ID, err) } usedIDs := container.SetOf(job.AttemptJobID) for _, sibling := range siblings { @@ -174,7 +169,7 @@ func expandDeferredMatrix(ctx context.Context, job *actions_model.ActionRunJob, // conditional update is an atomic claim: only the caller that flips IsMatrixDeferred inserts. beforeClaim := *job if err := applyCombo(job, expandedJobs[0]); err != nil { - return failTerminal(err) + return nil, failTerminal(err) } job.IsMatrixDeferred = false affected, err := actions_model.UpdateRunJob(ctx, job, @@ -209,13 +204,9 @@ func expandDeferredMatrix(ctx context.Context, job *actions_model.ActionRunJob, // restoreDeferredMatrixPlaceholder rewinds a rerun clone of a dynamic-matrix combination into the unexpanded placeholder it grew from func restoreDeferredMatrixPlaceholder(clone *actions_model.ActionRunJob) error { - var swf jobparser.SingleWorkflow - if err := yaml.Unmarshal(clone.DeferredMatrixPayload, &swf); err != nil { - return fmt.Errorf("unmarshal deferred matrix payload: %w", err) - } - _, parsed := swf.Job() - if parsed == nil { - return errors.New("deferred matrix payload contains no job") + _, parsed, err := jobparser.ParseRawSingleWorkflow(clone.DeferredMatrixPayload) + if err != nil { + return fmt.Errorf("parse deferred matrix payload: %w", err) } clone.Name = util.EllipsisDisplayString(parsed.Name, 255) clone.WorkflowPayload = slices.Clone(clone.DeferredMatrixPayload) diff --git a/services/actions/matrix_test.go b/services/actions/matrix_test.go index 853a886a55..aac0d47720 100644 --- a/services/actions/matrix_test.go +++ b/services/actions/matrix_test.go @@ -142,6 +142,7 @@ func TestExpandDeferredMatrix(t *testing.T) { for _, tt := range []struct { name, matrixValue string outputs map[string]string + prepare func(t *testing.T, job *actions_model.ActionRunJob) }{ {name: "unresolvable matrix", matrixValue: "${{ fromJson(needs.generate.outputs.missing) }}"}, { @@ -151,6 +152,16 @@ func TestExpandDeferredMatrix(t *testing.T) { // the rows the setup plants, plus the placeholder the first combination reuses. outputs: map[string]string{"many": "[" + strings.Repeat("0,", actions_model.MaxJobNumPerRun-2) + "0]"}, }, + { + // A malformed payload fails the same way on every pass, so returning it would requeue the + // run forever instead of ever reaching a terminal status. + name: "unreadable caller inputs", + matrixValue: "${{ fromJson(needs.generate.outputs.values) }}", + prepare: func(t *testing.T, job *actions_model.ActionRunJob) { + _, err := db.Exec(t.Context(), "UPDATE `action_run_job` SET call_payload = ? WHERE id = ?", "{", job.ParentJobID) + require.NoError(t, err) + }, + }, } { t.Run(tt.name+" fails the job", func(t *testing.T) { outputs := tt.outputs @@ -158,6 +169,9 @@ func TestExpandDeferredMatrix(t *testing.T) { outputs = map[string]string{"values": `["a","b","c"]`} } job := setupDeferredMatrixJob(t, tt.matrixValue, "", outputs) + if tt.prepare != nil { + tt.prepare(t, job) + } siblings, err := expandDeferredMatrix(t.Context(), job, nil) require.NoError(t, err) @@ -191,7 +205,13 @@ func TestDeferredMatrixResolverGating(t *testing.T) { {name: "skipped need", needStatus: actions_model.StatusSkipped, wantBuilds: []string{"build"}}, { name: "`if:` gated per combination", needStatus: actions_model.StatusSuccess, - jobIf: "${{ matrix.value != 'a' }}", outputs: map[string]string{"values": `["a","b"]`}, + jobIf: "${{ matrix.value == 'b' }}", outputs: map[string]string{"values": `["a","b"]`}, + wantBuilds: []string{"build (a)", "build (b)"}, + }, + { + // The same gate without the `${{ }}`, which must expand rather than skip the whole job. + name: "brace-less `if:` gated per combination", needStatus: actions_model.StatusSuccess, + jobIf: "matrix.value == 'b'", outputs: map[string]string{"values": `["a","b"]`}, wantBuilds: []string{"build (a)", "build (b)"}, }, } { @@ -218,3 +238,34 @@ func TestDeferredMatrixResolverGating(t *testing.T) { }) } } + +func TestDeferredMatrixResolverDefersDependents(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + ctx := t.Context() + + // `build (a)` is skipped by the `if:` once it carries its own combination, `build (b)` runs. + build := setupDeferredMatrixJob(t, "${{ fromJson(needs.generate.outputs.values) }}", + "${{ matrix.value != 'a' }}", map[string]string{"values": `["a","b"]`}) + + attemptJobID, err := actions_model.GetNextAttemptJobID(ctx, build.RunID) + require.NoError(t, err) + report := &actions_model.ActionRunJob{ + RunID: build.RunID, RunAttemptID: build.RunAttemptID, AttemptJobID: attemptJobID, + RepoID: build.RepoID, OwnerID: build.OwnerID, ParentJobID: build.ParentJobID, + JobID: "report", Name: "report", Status: actions_model.StatusBlocked, + Needs: []string{"build"}, WorkflowPayload: minimalWorkflowPayload("report"), + } + require.NoError(t, db.Insert(ctx, report)) + + jobs := runJobs(t, build.RunID, build.RunAttemptID) + require.NoError(t, jobs.LoadRuns(ctx, false)) + updates, err := newJobStatusResolver(jobs, nil).Resolve(ctx) + require.NoError(t, err) + + assert.Equal(t, actions_model.StatusSkipped, updates[build.ID], "the combination the `if:` excludes") + assert.NotContains(t, updates, report.ID, "report must wait for the re-emit, which sees `build (b)` too") + + // The sibling the pass would otherwise have resolved report against is there, and still to run. + sibling := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: build.RunID, Name: "build (b)"}) + assert.Equal(t, actions_model.StatusBlocked, sibling.Status) +} diff --git a/services/actions/max_parallel.go b/services/actions/max_parallel.go index 16be7b0356..703a11385b 100644 --- a/services/actions/max_parallel.go +++ b/services/actions/max_parallel.go @@ -6,6 +6,7 @@ package actions import ( "math" "strconv" + "strings" actions_model "gitea.dev/models/actions" "gitea.dev/modules/log" @@ -20,7 +21,15 @@ func parseMaxParallel(jobID, maxParallelString string) int { } maxParallel, err := strconv.ParseFloat(maxParallelString, 64) if err != nil || math.IsNaN(maxParallel) { - log.Debug("job %s: unsupported max-parallel value %q, treating as unlimited", jobID, maxParallelString) + // Both fall back to unlimited, but an expression is a gap in Gitea while a non-number is the + // author's mistake, so dropping the cap must not be reported the same way. + if strings.Contains(maxParallelString, "${{") { + // TODO: evaluate it against the contexts `if:` and the matrix already resolve, so that an + // expression can actually cap a job instead of quietly disabling the cap. + log.Debug("job %s: max-parallel %q is an expression, which is not evaluated yet: treating as unlimited", jobID, maxParallelString) + } else { + log.Warn("job %s: max-parallel %q is not a number, treating as unlimited", jobID, maxParallelString) + } return 0 } // a run can never hold more jobs than MaxJobNumPerRun, so clamping there keeps the cast total diff --git a/services/actions/max_parallel_test.go b/services/actions/max_parallel_test.go index c7c45e4508..b571891066 100644 --- a/services/actions/max_parallel_test.go +++ b/services/actions/max_parallel_test.go @@ -29,8 +29,8 @@ func TestParseMaxParallel(t *testing.T) { {"-1.5", 0}, // truncates to -1, which means unlimited {"1e3", 256}, // clamped to MaxJobNumPerRun {"nan", 0}, // must not reach the int cast - {"${{ vars.n }}", 0}, // expressions are not evaluated yet - {"abc", 0}, + {"${{ vars.n }}", 0}, // expressions are not evaluated yet, logged as such + {"abc", 0}, // a plain workflow error, warned about rather than hidden } for _, tt := range tests { assert.Equal(t, tt.want, parseMaxParallel("job", tt.input), "input %q", tt.input) diff --git a/services/actions/rerun.go b/services/actions/rerun.go index 7aee11ba89..a89bd9cb5e 100644 --- a/services/actions/rerun.go +++ b/services/actions/rerun.go @@ -653,11 +653,14 @@ func (p *rerunPlan) collectMatrixCollapse() { if p.skipCloneTemplateJobIDs.Contains(anchor.ID) { continue } - inRerunSet := slices.ContainsFunc(rows, func(j *actions_model.ActionRunJob) bool { - return p.rerunAttemptJobIDs.Contains(j.AttemptJobID) - }) - if !inRerunSet { - continue // pass-through group, cloned as-is + // Gate on the anchor rather than on any row of the group: execRerunPlan only resets a row whose + // own AttemptJobID is in the rerun set, so restoring the placeholder onto an anchor it treats + // as pass-through would clone a row that keeps its old terminal status while carrying the raw, + // unexpanded payload, and nothing expands a job that is not blocked. An anchor can be left + // out of the rerun set while a sibling is in it: partially re-running the subtree of a + // matrix-expanded reusable caller puts that combination in ancestorAttemptJobIDs instead. + if !p.rerunAttemptJobIDs.Contains(anchor.AttemptJobID) { + continue // pass-through anchor, the group is cloned as-is } unexpanded := len(rows) == 1 && anchor.IsMatrixDeferred if !unexpanded && !p.hasRerunDependency(anchor) { diff --git a/services/actions/rerun_test.go b/services/actions/rerun_test.go index b4cb4ad734..5cfeff0bb0 100644 --- a/services/actions/rerun_test.go +++ b/services/actions/rerun_test.go @@ -363,3 +363,93 @@ func rowIDsOf(jobs ...*actions_model.ActionRunJob) []int64 { } return out } + +func TestCollectMatrixCollapse(t *testing.T) { + // A dynamic-matrix group is the anchor (the combination that kept DeferredMatrixPayload) plus its + // sibling combinations. Collapsing it rewinds the anchor to an unexpanded placeholder and drops + // the siblings, so the new attempt re-derives the matrix from the fresh needs outputs. + matrixRow := func(id, attemptJobID int64, jobID string, parentID int64, isAnchor, isCaller bool, needs ...string) *actions_model.ActionRunJob { + job := templateJob(id, attemptJobID, jobID, parentID, isCaller, needs...) + if isAnchor { + job.DeferredMatrixPayload = []byte("name: t\non: push\njobs:\n build:\n steps: [{run: echo}]\n") + } + return job + } + + t.Run("a rerun of the needs collapses the group", func(t *testing.T) { + generate := templateJob(101, 1, "generate", 0, false) + build1 := matrixRow(102, 2, "build", 0, true, false, "generate") + build2 := matrixRow(103, 3, "build", 0, false, false, "generate") + plan := &rerunPlan{templateJobs: []*actions_model.ActionRunJob{generate, build1, build2}} + require.NoError(t, plan.expandRerunJobIDs([]*actions_model.ActionRunJob{generate})) + plan.skipCloneTemplateJobIDs = plan.collectResetCallerDescendants() + plan.collectMatrixCollapse() + + assert.ElementsMatch(t, rowIDsOf(build1), plan.matrixPlaceholderTemplateIDs.Values()) + assert.ElementsMatch(t, rowIDsOf(build2), plan.matrixSiblingSkipTemplateIDs.Values()) + }) + + t.Run("re-running only a combination keeps the group as it is", func(t *testing.T) { + // generate is not re-run, so its outputs still stand and the combinations stay valid. + generate := templateJob(101, 1, "generate", 0, false) + build1 := matrixRow(102, 2, "build", 0, true, false, "generate") + build2 := matrixRow(103, 3, "build", 0, false, false, "generate") + plan := &rerunPlan{templateJobs: []*actions_model.ActionRunJob{generate, build1, build2}} + require.NoError(t, plan.expandRerunJobIDs([]*actions_model.ActionRunJob{build2})) + plan.skipCloneTemplateJobIDs = plan.collectResetCallerDescendants() + plan.collectMatrixCollapse() + + assert.Empty(t, plan.matrixPlaceholderTemplateIDs) + assert.Empty(t, plan.matrixSiblingSkipTemplateIDs) + }) + + t.Run("a pass-through anchor is never rewound", func(t *testing.T) { + // build is a matrix-expanded reusable caller. Re-running a job inside build (1)'s subtree + // together with generate leaves build (1) an ancestor rather than a rerun job, while build (2) + // does join the rerun set. execRerunPlan clones an ancestor with its old terminal status, so + // restoring the placeholder onto it would leave a done job holding the raw, unexpanded payload + // that nothing ever expands - and drop build (2) on top of it. + generate := templateJob(101, 1, "generate", 0, false) + build1 := matrixRow(102, 2, "build", 0, true, true, "generate") + build2 := matrixRow(103, 3, "build", 0, false, true, "generate") + inner := templateJob(104, 4, "inner", 102, false) + plan := &rerunPlan{templateJobs: []*actions_model.ActionRunJob{generate, build1, build2, inner}} + require.NoError(t, plan.expandRerunJobIDs([]*actions_model.ActionRunJob{inner, generate})) + plan.skipCloneTemplateJobIDs = plan.collectResetCallerDescendants() + + require.Contains(t, plan.ancestorAttemptJobIDs, build1.AttemptJobID) + require.NotContains(t, plan.rerunAttemptJobIDs, build1.AttemptJobID) + require.Contains(t, plan.rerunAttemptJobIDs, build2.AttemptJobID) + + plan.collectMatrixCollapse() + assert.Empty(t, plan.matrixPlaceholderTemplateIDs) + assert.Empty(t, plan.matrixSiblingSkipTemplateIDs) + }) + + t.Run("an unexpanded placeholder is rewound whenever it is re-run", func(t *testing.T) { + // The previous attempt never got to expand it, so there is nothing to reuse. + generate := templateJob(101, 1, "generate", 0, false) + build := matrixRow(102, 2, "build", 0, true, false, "generate") + build.IsMatrixDeferred = true + plan := &rerunPlan{templateJobs: []*actions_model.ActionRunJob{generate, build}} + require.NoError(t, plan.expandRerunJobIDs([]*actions_model.ActionRunJob{build})) + plan.skipCloneTemplateJobIDs = plan.collectResetCallerDescendants() + plan.collectMatrixCollapse() + + assert.ElementsMatch(t, rowIDsOf(build), plan.matrixPlaceholderTemplateIDs.Values()) + assert.Empty(t, plan.matrixSiblingSkipTemplateIDs) + }) + + t.Run("a plan-time matrix is left alone", func(t *testing.T) { + generate := templateJob(101, 1, "generate", 0, false) + build1 := matrixRow(102, 2, "build", 0, false, false, "generate") + build2 := matrixRow(103, 3, "build", 0, false, false, "generate") + plan := &rerunPlan{templateJobs: []*actions_model.ActionRunJob{generate, build1, build2}} + require.NoError(t, plan.expandRerunJobIDs(nil)) + plan.skipCloneTemplateJobIDs = plan.collectResetCallerDescendants() + plan.collectMatrixCollapse() + + assert.Empty(t, plan.matrixPlaceholderTemplateIDs) + assert.Empty(t, plan.matrixSiblingSkipTemplateIDs) + }) +} diff --git a/tests/integration/actions_dynamic_matrix_test.go b/tests/integration/actions_dynamic_matrix_test.go index 7c3b6341c4..657e8e3f7d 100644 --- a/tests/integration/actions_dynamic_matrix_test.go +++ b/tests/integration/actions_dynamic_matrix_test.go @@ -26,6 +26,10 @@ import ( // planned as a single placeholder and expanded once its dependency completes. `build` exercises the // expansion, `report` that a downstream job sees the combinations' outputs, `gated` and `partial` the // `if:` gates on either side of it, and `static` that a matrix expanded at plan time is left alone. +// `included` covers the placeholder shapes that only survive as long as nothing re-parses their +// payload: `include:` is still a scalar there, which act refuses to read at all. +// `partial` and `strict` gate on `matrix.*` from either direction: neither may be decided before the +// combinations exist, or the whole job is skipped instead of the combinations the gate excludes. // A full rerun then re-derives the matrix from the new attempt's outputs instead of reusing the // previous combinations, keeping the AttemptJobID of every combination that recurs. func TestDynamicMatrixEvaluation(t *testing.T) { @@ -47,9 +51,12 @@ jobs: runs-on: ubuntu-latest outputs: matrix: ${{ steps.set.outputs.matrix }} + combos: ${{ steps.set.outputs.combos }} steps: - id: set - run: echo "matrix=[1,2]" >> "$GITHUB_OUTPUT" + run: | + echo "matrix=[1,2]" >> "$GITHUB_OUTPUT" + echo 'combos=[{"name":"x"},{"name":"y"}]' >> "$GITHUB_OUTPUT" build: needs: [generate] runs-on: ubuntu-latest @@ -69,6 +76,14 @@ jobs: os: [a, b] steps: - run: echo "${{ matrix.os }}" + included: + needs: [generate] + runs-on: ubuntu-latest + strategy: + matrix: + include: ${{ fromJson(needs.generate.outputs.combos) }} + steps: + - run: echo "${{ matrix.name }}" gated: needs: [generate] if: false @@ -87,6 +102,15 @@ jobs: value: ${{ fromJson(needs.generate.outputs.matrix) }} steps: - run: echo "${{ matrix.value }}" + strict: + needs: [generate] + if: matrix.value == 1 + runs-on: ubuntu-latest + strategy: + matrix: + value: ${{ fromJson(needs.generate.outputs.matrix) }} + steps: + - run: echo "${{ matrix.value }}" report: needs: [build] runs-on: ubuntu-latest @@ -101,7 +125,7 @@ jobs: _, _, run := getTaskAndJobAndRunByTaskID(t, generateTask.Id) runner.execTask(t, generateTask, &mockTaskOutcome{ result: runnerv1.Result_RESULT_SUCCESS, - outputs: map[string]string{"matrix": "[1,2]"}, + outputs: map[string]string{"matrix": "[1,2]", "combos": `[{"name":"x"},{"name":"y"}]`}, }) // execAttempt runs the attempt's remaining tasks, recording each job's name and AttemptJobID(order is not fixed). @@ -127,13 +151,20 @@ jobs: return names } - seen := execAttempt(t, 6) + seen := execAttempt(t, 9) firstAttemptIDs := maps.Clone(attemptJobIDs) - // `gated` is decided before the matrix is touched, so it never expands and is skipped as one job; - // `partial (1)` is decided afterwards, against its own combination. - assert.ElementsMatch(t, []string{"build (1)", "build (2)", "static (a)", "static (b)", "partial (2)", "report"}, seen) - skipped := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: run.ID, Name: "partial (1)"}) - assert.Equal(t, actions_model.StatusSkipped, skipped.Status) + // `gated` is decided before the matrix is touched, so it never expands and is skipped as one job; + // `partial (1)` and `strict (2)` are decided afterwards, each against its own combination. + assert.ElementsMatch(t, []string{ + "build (1)", "build (2)", "included (x)", "included (y)", + "static (a)", "static (b)", "partial (2)", "strict (1)", "report", + }, seen) + for _, name := range []string{"partial (1)", "strict (2)"} { + skipped := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{RunID: run.ID, Name: name}) + assert.Equal(t, actions_model.StatusSkipped, skipped.Status) + } + // A gate reading `matrix.*` must not be decided against the unexpanded placeholder. + unittest.AssertNotExistsBean(t, &actions_model.ActionRunJob{RunID: run.ID, Name: "strict"}) buildNeed, ok := reportTask.Needs["build"] require.True(t, ok, "report must see the expanded combinations as its 'build' need") @@ -149,14 +180,16 @@ jobs: require.Equal(t, "generate", getTaskJobNameByTaskID(t, token, user2.Name, apiRepo.Name, generateTask2.Id)) assert.Equal(t, "2", generateTask2.Context.GetFields()["run_attempt"].GetStringValue()) runner.execTask(t, generateTask2, &mockTaskOutcome{ - result: runnerv1.Result_RESULT_SUCCESS, - outputs: map[string]string{"matrix": "[1,2,3]"}, + result: runnerv1.Result_RESULT_SUCCESS, + // `combos` is unchanged, so both `included` combinations recur and have to keep their AttemptJobIDs. + outputs: map[string]string{"matrix": "[1,2,3]", "combos": `[{"name":"x"},{"name":"y"}]`}, }) // The matrix now yields a third value, while `static` is cloned as-is rather than collapsed. - seenRerun := execAttempt(t, 8) + seenRerun := execAttempt(t, 11) assert.ElementsMatch(t, []string{ - "build (1)", "build (2)", "build (3)", "static (a)", "static (b)", "partial (2)", "partial (3)", "report", + "build (1)", "build (2)", "build (3)", "included (x)", "included (y)", + "static (a)", "static (b)", "partial (2)", "partial (3)", "strict (1)", "report", }, seenRerun) for name, firstID := range firstAttemptIDs { assert.Equal(t, firstID, attemptJobIDs[name], "%s keeps its AttemptJobID across attempts", name)