From 9d6ae1471eee19109d1250ce5294439cfba073d0 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Fri, 2 Jan 2026 15:26:11 +0100 Subject: [PATCH] fix: re-running an expanded reusable workflow causes duplicate "attempt 1" job (#10666) Placeholder tasks, which are used to store the outputs of a reusable workflow, were hard coded to always have attempt 1. If you executed "Re-run all jobs" with a reusable workflow, a second placeholder task would be created with the same attempt, which caused: (a) Forgejo to not know which attempt, and therefore which outputs, were valid, and (b) the UI to be stuck in "You are viewing an out-of-date run of this job..." when viewing the job. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [x] I do not want this change to show in the release notes. - [ ] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10666 Reviewed-by: Andreas Ahlenstorf Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- models/actions/task.go | 8 ++-- models/actions/task_test.go | 2 +- .../action_run.yml | 20 +++++++++ .../action_run_job.yml | 36 ++++++++++++++++ .../action_task.yml | 12 ++++++ .../action_task_output.yml | 43 +++++++++++++++++++ services/actions/job_emitter.go | 6 ++- services/actions/job_emitter_test.go | 29 ++++++++++--- 8 files changed, 145 insertions(+), 11 deletions(-) create mode 100644 services/actions/Test_tryHandleWorkflowCallOuterJob/action_task.yml diff --git a/models/actions/task.go b/models/actions/task.go index 517beee999..45f1643989 100644 --- a/models/actions/task.go +++ b/models/actions/task.go @@ -421,12 +421,14 @@ func CreateTaskForRunner(ctx context.Context, runner *ActionRunner) (*ActionTask return task, true, nil } -// Placeholder tasks are created when the status/content of an `ActionRunJob` is resolved by Forgejo without dispatch to -// a runner, specifically in the case of a workflow call's outer job. +// Placeholder tasks are created when the status/content of an [ActionRunJob] is resolved by Forgejo without dispatch to +// a runner, specifically in the case of a workflow call's outer job. It is the responsibility of the caller to +// increment the job's Attempt field before invoking this method, and to update that field in the database, so that +// reruns can function for placeholder tasks and provide updated outputs. func CreatePlaceholderTask(ctx context.Context, job *ActionRunJob, outputs map[string]string) (*ActionTask, error) { actionTask := &ActionTask{ JobID: job.ID, - Attempt: 1, + Attempt: job.Attempt, Started: timeutil.TimeStampNow(), Stopped: timeutil.TimeStampNow(), Status: job.Status, diff --git a/models/actions/task_test.go b/models/actions/task_test.go index 831a8bc3dc..08006e6b7b 100644 --- a/models/actions/task_test.go +++ b/models/actions/task_test.go @@ -58,7 +58,7 @@ func TestActionTask_CreatePlaceholderTask(t *testing.T) { assert.NotEqualValues(t, 0, task.ID) assert.Equal(t, job.ID, task.JobID) - assert.EqualValues(t, 1, task.Attempt) + assert.EqualValues(t, 0, task.Attempt) assert.NotEqualValues(t, 0, task.Started) assert.NotEqualValues(t, 0, task.Stopped) assert.Equal(t, job.Status, task.Status) diff --git a/services/actions/Test_tryHandleWorkflowCallOuterJob/action_run.yml b/services/actions/Test_tryHandleWorkflowCallOuterJob/action_run.yml index 0cb546b3aa..7d4a59b0a3 100644 --- a/services/actions/Test_tryHandleWorkflowCallOuterJob/action_run.yml +++ b/services/actions/Test_tryHandleWorkflowCallOuterJob/action_run.yml @@ -17,3 +17,23 @@ updated: 1683636626 need_approval: 0 approved_by: 0 + +# Case w/ action_run_job.id = 603 +- + id: 901 + title: "running workflow_dispatch run" + owner_id: 2 + repo_id: 63 + workflow_id: "running.yaml" + index: 5 + trigger_user_id: 2 + ref: "refs/heads/main" + commit_sha: "97f29ee599c373c729132a5c46a046978311e0ee" + trigger_event: "workflow_dispatch" + is_fork_pull_request: 0 + status: 6 # running + started: 1683636528 + created: 1683636108 + updated: 1683636626 + need_approval: 0 + approved_by: 0 diff --git a/services/actions/Test_tryHandleWorkflowCallOuterJob/action_run_job.yml b/services/actions/Test_tryHandleWorkflowCallOuterJob/action_run_job.yml index c0e55fd1a4..03449040ca 100644 --- a/services/actions/Test_tryHandleWorkflowCallOuterJob/action_run_job.yml +++ b/services/actions/Test_tryHandleWorkflowCallOuterJob/action_run_job.yml @@ -48,3 +48,39 @@ status: 1 # success job_id: outer-job.inner-job task_id: 100 + +# Case w/ action_run_job.id = 603 -- functionality of this is a duplicate of case 601 but it has already been executed +# (attempt = 1) and we're testing that the updated outputs from the rerun are stored. +- + id: 603 + run_id: 901 + attempt: 1 + status: 1 # success + started: 1683636528 + needs: ["outer-job.inner-job"] + workflow_payload: | + "on": + push: + jobs: + outer-job: + name: outer-job + if: false + uses: ./.forgjeo/workflows/reusable.yml + outputs: + from_inner_job: ${{ needs['outer-job.inner-job'].outputs.my_output }} + from_inner_job_result: ${{ needs['outer-job.inner-job'].result }} + from_forgejo_ctx: ${{ forgejo.ref }} + from_input_ctx: ${{ inputs.wc_input }}! + from_vars_repo: ${{ vars.repo_var }} + from_vars_org: ${{ vars.org_var }} + from_vars_global: ${{ vars.global_var }} + __metadata: + workflow_call_inputs: + wc_input: 'hello, world' + workflow_call_id: b5a9f46f1f2513d7777fde50b169d323a6519e349cc175484c947ac315a209ed +- # inner job of run 603 + id: 604 + run_id: 901 + status: 1 # success + job_id: outer-job.inner-job + task_id: 101 diff --git a/services/actions/Test_tryHandleWorkflowCallOuterJob/action_task.yml b/services/actions/Test_tryHandleWorkflowCallOuterJob/action_task.yml new file mode 100644 index 0000000000..8781278efd --- /dev/null +++ b/services/actions/Test_tryHandleWorkflowCallOuterJob/action_task.yml @@ -0,0 +1,12 @@ +# Case w/ action_run_job.id = 603 -- this is the placeholder task from attempt 1 +- + id: 58 + attempt: 1 + runner_id: 1 + status: 1 # success + started: 1683636528 + stopped: 1683636626 + repo_id: 63 + owner_id: 2 + commit_sha: 97f29ee599c373c729132a5c46a046978311e0ee + is_fork_pull_request: false diff --git a/services/actions/Test_tryHandleWorkflowCallOuterJob/action_task_output.yml b/services/actions/Test_tryHandleWorkflowCallOuterJob/action_task_output.yml index 9bf82649c1..1383c5be16 100644 --- a/services/actions/Test_tryHandleWorkflowCallOuterJob/action_task_output.yml +++ b/services/actions/Test_tryHandleWorkflowCallOuterJob/action_task_output.yml @@ -4,3 +4,46 @@ task_id: 100 output_key: my_output output_value: 'abcdefghijklmnopqrstuvwxyz' + +# Case w/ action_run_job.id = 603 -- outputs from a previous attempt +- + id: 101 + task_id: 58 + output_key: from_inner_job + output_value: obsolete value +- + id: 102 + task_id: 58 + output_key: from_inner_job_result + output_value: obsolete value +- + id: 103 + task_id: 58 + output_key: from_forgejo_ctx + output_value: obsolete value +- + id: 104 + task_id: 58 + output_key: from_input_ctx + output_value: obsolete value +- + id: 105 + task_id: 58 + output_key: from_vars_repo + output_value: obsolete value +- + id: 106 + task_id: 58 + output_key: from_vars_org + output_value: obsolete value +- + id: 107 + task_id: 58 + output_key: from_vars_global + output_value: obsolete value +# Case w/ action_run_job.id = 603 -- input from another job... +- + id: 108 + task_id: 101 + output_key: my_output + output_value: 'abcdefghijklmnopqrstuvwxyz' diff --git a/services/actions/job_emitter.go b/services/actions/job_emitter.go index f1e9d21cbb..389b19ac92 100644 --- a/services/actions/job_emitter.go +++ b/services/actions/job_emitter.go @@ -523,12 +523,14 @@ func tryHandleWorkflowCallOuterJob(ctx context.Context, job *actions_model.Actio ) // Insert a placeholder task with all the computed outputs + job.Attempt++ actionTask, err := actions_model.CreatePlaceholderTask(ctx, job, outputs) if err != nil { return nil, fmt.Errorf("failure to insert placeholder task: %w", err) } - // Populate task_id and ask caller to update it in DB: + // Populate task_id and ask caller to update it in DB. + // Update previously incremented attempt field as well. job.TaskID = actionTask.ID - return []string{"task_id"}, nil + return []string{"task_id", "attempt"}, nil } diff --git a/services/actions/job_emitter_test.go b/services/actions/job_emitter_test.go index 767d5bdca4..6b8ccca6f9 100644 --- a/services/actions/job_emitter_test.go +++ b/services/actions/job_emitter_test.go @@ -672,10 +672,11 @@ func Test_tryHandleIncompleteMatrix(t *testing.T) { func Test_tryHandleWorkflowCallOuterJob(t *testing.T) { tests := []struct { - name string - runJobID int64 - updateFields []string - outputs map[string]string + name string + runJobID int64 + updateFields []string + outputs map[string]string + expectedAttempt int }{ { name: "not workflow call outer job", @@ -684,7 +685,7 @@ func Test_tryHandleWorkflowCallOuterJob(t *testing.T) { { name: "outputs for every context", runJobID: 601, - updateFields: []string{"task_id"}, + updateFields: []string{"task_id", "attempt"}, outputs: map[string]string{ "from_inner_job": "abcdefghijklmnopqrstuvwxyz", "from_inner_job_result": "success", @@ -694,6 +695,22 @@ func Test_tryHandleWorkflowCallOuterJob(t *testing.T) { "from_vars_org": "this is an org variable", "from_vars_global": "this is a global variable", }, + expectedAttempt: 1, + }, + { + name: "attempt 2 rerun task", + runJobID: 603, + updateFields: []string{"task_id", "attempt"}, + outputs: map[string]string{ + "from_inner_job": "abcdefghijklmnopqrstuvwxyz", + "from_inner_job_result": "success", + "from_forgejo_ctx": "refs/heads/main", + "from_input_ctx": "hello, world!", + "from_vars_repo": "this is a repo variable", + "from_vars_org": "this is an org variable", + "from_vars_global": "this is a global variable", + }, + expectedAttempt: 2, }, } for _, tt := range tests { @@ -709,6 +726,8 @@ func Test_tryHandleWorkflowCallOuterJob(t *testing.T) { assert.Equal(t, tt.updateFields, updateFields) if tt.updateFields != nil { + assert.EqualValues(t, tt.expectedAttempt, outerJob.Attempt) + // TaskID expected to be set by tryHandleWorkflowCallOuterJob require.NotEqualValues(t, 0, outerJob.TaskID)