kyush/forgejo
1
0
Fork 0
forked from mirrors/forgejo
Code Pull requests Activity

fix: re-running an expanded reusable workflow causes duplicate "attempt 1" job (#10666)

Browse source 
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/<pull request number>.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 <aahlenst@noreply.codeberg.org>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
This commit is contained in:
Mathieu Fenniak 2026-01-02 15:26:11 +01:00 • committed by Mathieu Fenniak
commit 9d6ae1471e
8 changed files with 145 additions and 11 deletions
Download patch file Download diff file Expand all files Collapse all files

8
models/actions/task.go
View file

@ -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,

2
models/actions/task_test.go
View file

@ -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)

20
services/actions/Test_tryHandleWorkflowCallOuterJob/action_run.yml
View file

@ -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

36
services/actions/Test_tryHandleWorkflowCallOuterJob/action_run_job.yml
View file

@ -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

12
services/actions/Test_tryHandleWorkflowCallOuterJob/action_task.yml Normal file
View file

@ -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

43
services/actions/Test_tryHandleWorkflowCallOuterJob/action_task_output.yml
View file

@ -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'

6
services/actions/job_emitter.go
View file

@ -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
}

29
services/actions/job_emitter_test.go
View file

@ -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)