From 1127aca2d2eab70b39ee0df0a01a456043067745 Mon Sep 17 00:00:00 2001 From: Andreas Ahlenstorf Date: Fri, 20 Mar 2026 17:23:09 +0100 Subject: [PATCH] fix: set attempt number of action run jobs eagerly (#11750) A Forgejo Action job should be uniquely identifiable by its `ID` and `Attempt` number. Each time a particular job is (re-)run, its `Attempt` number is incremented while its `ID` remains static. Unfortunately, `Attempt` is not incremented when the (re-)run is triggered, but right when Forgejo Runner requests the job. That makes identifying a particular run much harder, because the attempt number is changed in the midst of an attempt. Furthermore, it requires taking the job's `Status` into account. This is fixed by setting the correct attempt number right when a (re-)run is triggered. That means that the `Attempt` number remains static for the duration of a single attempt. ## 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 for Go changes (can be removed for JavaScript changes) - 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 ran... - [x] `make pr-go` before pushing ### Tests for JavaScript changes (can be removed for Go changes) - 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. - [ ] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. *The decision if the pull request will be shown in the release notes is up to the mergers / release team.* The content of the `release-notes/.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11750 Reviewed-by: Mathieu Fenniak Co-authored-by: Andreas Ahlenstorf Co-committed-by: Andreas Ahlenstorf --- models/actions/run.go | 11 +++- models/actions/run_job.go | 14 +++++ models/actions/run_job_test.go | 37 +++++++++++ models/actions/run_test.go | 61 +++++++++++++++++++ models/actions/task.go | 1 - .../actions/TestActionsRerun/action_run.yml | 19 ++++++ .../TestActionsRerun/action_run_job.yml | 58 ++++++++++++++++++ routers/web/repo/actions/view.go | 19 ++---- routers/web/repo/actions/view_test.go | 13 ++-- 9 files changed, 211 insertions(+), 22 deletions(-) create mode 100644 routers/web/repo/actions/TestActionsRerun/action_run.yml create mode 100644 routers/web/repo/actions/TestActionsRerun/action_run_job.yml diff --git a/models/actions/run.go b/models/actions/run.go index c4e4a40eb2..88e8908fed 100644 --- a/models/actions/run.go +++ b/models/actions/run.go @@ -383,7 +383,8 @@ func InsertRunJobs(ctx context.Context, run *ActionRun, jobs []*jobparser.Single name, _ = util.SplitStringAtByteN(job.Name, 255) runsOn = job.RunsOn() } - runJobs = append(runJobs, &ActionRunJob{ + + runJob := &ActionRunJob{ RunID: run.ID, RepoID: run.RepoID, OwnerID: run.OwnerID, @@ -394,8 +395,12 @@ func InsertRunJobs(ctx context.Context, run *ActionRun, jobs []*jobparser.Single JobID: id, Needs: needs, RunsOn: runsOn, - Status: status, - }) + } + if err := runJob.PrepareNextAttempt(status); err != nil { + return err + } + + runJobs = append(runJobs, runJob) } if len(runJobs) > 0 { diff --git a/models/actions/run_job.go b/models/actions/run_job.go index 926eb9f0de..a9fbdfaed4 100644 --- a/models/actions/run_job.go +++ b/models/actions/run_job.go @@ -117,6 +117,20 @@ func (job *ActionRunJob) ItRunsOn(labels []string) bool { return labelSet.IsSubset(job.RunsOn) } +func (job *ActionRunJob) PrepareNextAttempt(initialStatus Status) error { + if job.Status != StatusUnknown && !job.Status.IsDone() { + return fmt.Errorf("cannot prepare next attempt because job %d is active: %s", job.ID, job.Status.String()) + } + + job.Attempt++ + job.Started = 0 + job.Stopped = 0 + job.TaskID = 0 + job.Status = initialStatus + + return nil +} + func GetRunJobByID(ctx context.Context, id int64) (*ActionRunJob, error) { var job ActionRunJob has, err := db.GetEngine(ctx).Where("id=?", id).Get(&job) diff --git a/models/actions/run_job_test.go b/models/actions/run_job_test.go index 90080473c7..945b2d150f 100644 --- a/models/actions/run_job_test.go +++ b/models/actions/run_job_test.go @@ -8,6 +8,7 @@ import ( "forgejo.org/models/db" "forgejo.org/models/unittest" + "forgejo.org/modules/timeutil" "code.forgejo.org/forgejo/runner/v12/act/jobparser" "github.com/stretchr/testify/assert" @@ -301,3 +302,39 @@ func TestRunHasOtherJobs(t *testing.T) { require.NoError(t, err) assert.False(t, has) } + +func TestActionRunJobPrepareNextAttempt(t *testing.T) { + job := ActionRunJob{ID: 46} + err := job.PrepareNextAttempt(StatusWaiting) + require.NoError(t, err) + + assert.Equal(t, int64(1), job.Attempt) + assert.Zero(t, job.Started) + assert.Zero(t, job.Stopped) + assert.Zero(t, job.TaskID) + assert.Equal(t, StatusWaiting, job.Status) + + job.Started = timeutil.TimeStampNow() + job.Stopped = timeutil.TimeStampNow() + job.TaskID = int64(59) + job.Status = StatusFailure + + err = job.PrepareNextAttempt(StatusBlocked) + require.NoError(t, err) + + assert.Equal(t, int64(2), job.Attempt) + assert.Zero(t, job.Started) + assert.Zero(t, job.Stopped) + assert.Zero(t, job.TaskID) + assert.Equal(t, StatusBlocked, job.Status) + + // The job hasn't finished yet. Preparing a next attempt should not be possible. It should be left untouched. + err = job.PrepareNextAttempt(StatusWaiting) + require.ErrorContains(t, err, "cannot prepare next attempt because job 46 is active: blocked") + + assert.Equal(t, int64(2), job.Attempt) + assert.Zero(t, job.Started) + assert.Zero(t, job.Stopped) + assert.Zero(t, job.TaskID) + assert.Equal(t, StatusBlocked, job.Status) +} diff --git a/models/actions/run_test.go b/models/actions/run_test.go index f027512c75..41703f91e1 100644 --- a/models/actions/run_test.go +++ b/models/actions/run_test.go @@ -502,3 +502,64 @@ func TestComputeRunStatus(t *testing.T) { assert.Contains(t, columns, "stopped") }) } + +func TestInsertRunJobs(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + pullRequestPosterID := int64(4) + repoID := int64(10) + pullRequestID := int64(2) + actionRun := &ActionRun{ + RepoID: repoID, + PullRequestID: pullRequestID, + PullRequestPosterID: pullRequestPosterID, + CommitSHA: "1421f75bc5474c69fdb1dc176bcb96d381f935dd", + } + + workflowRaw := []byte(` +jobs: + build: + runs-on: fedora + test: + runs-on: debian + steps: [] +`) + jobs, err := jobparser.Parse(workflowRaw, false) + require.NoError(t, err) + + require.NoError(t, InsertRun(t.Context(), actionRun, jobs)) + + insertedJobs, err := db.Find[ActionRunJob](t.Context(), FindRunJobOptions{RunID: actionRun.ID}) + require.NoError(t, err) + require.Len(t, insertedJobs, 2) + + assert.Equal(t, actionRun.ID, insertedJobs[0].RunID) + assert.Equal(t, actionRun.RepoID, insertedJobs[0].RepoID) + assert.Equal(t, actionRun.OwnerID, insertedJobs[0].OwnerID) + assert.Equal(t, actionRun.CommitSHA, insertedJobs[0].CommitSHA) + assert.Equal(t, actionRun.IsForkPullRequest, insertedJobs[0].IsForkPullRequest) + assert.Equal(t, "build", insertedJobs[0].Name) + assert.Equal(t, "build", insertedJobs[0].JobID) + assert.Empty(t, insertedJobs[0].Needs) + assert.Equal(t, []string{"fedora"}, insertedJobs[0].RunsOn) + assert.Equal(t, int64(1), insertedJobs[0].Attempt) + assert.Zero(t, insertedJobs[0].Started) + assert.Zero(t, insertedJobs[0].Stopped) + assert.Zero(t, insertedJobs[0].TaskID) + assert.Equal(t, StatusWaiting, insertedJobs[0].Status) + + assert.Equal(t, actionRun.ID, insertedJobs[1].RunID) + assert.Equal(t, actionRun.RepoID, insertedJobs[1].RepoID) + assert.Equal(t, actionRun.OwnerID, insertedJobs[1].OwnerID) + assert.Equal(t, actionRun.CommitSHA, insertedJobs[1].CommitSHA) + assert.Equal(t, actionRun.IsForkPullRequest, insertedJobs[1].IsForkPullRequest) + assert.Equal(t, "test", insertedJobs[1].Name) + assert.Equal(t, "test", insertedJobs[1].JobID) + assert.Empty(t, insertedJobs[1].Needs) + assert.Equal(t, []string{"debian"}, insertedJobs[1].RunsOn) + assert.Equal(t, int64(1), insertedJobs[1].Attempt) + assert.Zero(t, insertedJobs[1].Started) + assert.Zero(t, insertedJobs[1].Stopped) + assert.Zero(t, insertedJobs[1].TaskID) + assert.Equal(t, StatusWaiting, insertedJobs[1].Status) +} diff --git a/models/actions/task.go b/models/actions/task.go index f67b5e58e1..6b169de3c0 100644 --- a/models/actions/task.go +++ b/models/actions/task.go @@ -378,7 +378,6 @@ func CreateTaskForRunner(ctx context.Context, runner *ActionRunner, requestKey * } now := timeutil.TimeStampNow() - job.Attempt++ job.Started = now job.Status = StatusRunning diff --git a/routers/web/repo/actions/TestActionsRerun/action_run.yml b/routers/web/repo/actions/TestActionsRerun/action_run.yml new file mode 100644 index 0000000000..9db0954b7c --- /dev/null +++ b/routers/web/repo/actions/TestActionsRerun/action_run.yml @@ -0,0 +1,19 @@ +- id: 83731 + title: "update actions" + repo_id: 1 + owner_id: 5 + workflow_id: "artifact.yaml" + index: 138574 + trigger_user_id: 1 + ref: "refs/heads/branch2" + commit_sha: "985f0301dba5e7b34be866819cd15ad3d8f508ee" + event: "push" + is_fork_pull_request: false + status: 1 # success + started: 1683636528 + stopped: 1683636626 + created: 1683636108 + updated: 1683636626 + need_approval: false + approved_by: 0 + event_payload: '{"head_commit":{"id":"5f22f7d0d95d614d25a5b68592adb345a4b5c7fd"}}' \ No newline at end of file diff --git a/routers/web/repo/actions/TestActionsRerun/action_run_job.yml b/routers/web/repo/actions/TestActionsRerun/action_run_job.yml new file mode 100644 index 0000000000..3d97591281 --- /dev/null +++ b/routers/web/repo/actions/TestActionsRerun/action_run_job.yml @@ -0,0 +1,58 @@ +- id: 248950 + run_id: 83731 + repo_id: 1 + owner_id: 1 + commit_sha: 985f0301dba5e7b34be866819cd15ad3d8f508ee + is_fork_pull_request: false + name: job_2 + attempt: 2 + job_id: job_2 + task_id: 47 + status: 1 + started: 1683636528 + stopped: 1683636626 + +- id: 248951 + run_id: 83731 + repo_id: 1 + owner_id: 1 + commit_sha: 985f0301dba5e7b34be866819cd15ad3d8f508ee + is_fork_pull_request: false + name: job_2 + attempt: 3 + job_id: job_2 + task_id: 47 + status: 1 + runs_on: '["ubuntu-latest"]' + started: 1683636528 + stopped: 1683636626 + +- id: 248952 + run_id: 83731 + repo_id: 1 + owner_id: 2 + commit_sha: 985f0301dba5e7b34be866819cd15ad3d8f508ee + is_fork_pull_request: false + name: job_2 + attempt: 5 + job_id: job_2 + task_id: 47 + status: 1 + runs_on: '["debian-latest"]' + started: 1683636528 + stopped: 1683636626 + +- id: 248953 + run_id: 83731 + repo_id: 1 + owner_id: 3 + commit_sha: 985f0301dba5e7b34be866819cd15ad3d8f508ee + is_fork_pull_request: false + name: job_2 + attempt: 2 + job_id: job_2 + task_id: 47 + status: 1 + runs_on: '["fedora"]' + started: 1683636528 + stopped: 1683636626 diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index caa35d37be..8deb733e16 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -520,11 +520,6 @@ func Rerun(ctx *app_context.Context) { return } if redirectURL == "" { - // ActionRunJob's `Attempt` field won't be updated to reflect the rerun until the job is picked by a - // runner. But we need to redirect the user somewhere; if they stay on the current attempt then the - // rerun's logs won't appear. So, we redirect to the upcoming new attempt and then we'll handle the - // weirdness in the UI if the attempt doesn't exist yet. - j.Attempt++ // note: this is intentionally not persisted redirectURL, err = j.HTMLURL(ctx) if err != nil { ctx.Error(http.StatusInternalServerError, err.Error()) @@ -552,8 +547,6 @@ func Rerun(ctx *app_context.Context) { return } if j.JobID == job.JobID { - // see earlier comment about redirectURL, applicable here as well - j.Attempt++ // note: this is intentionally not persisted redirectURL, err = j.HTMLURL(ctx) if err != nil { ctx.Error(http.StatusInternalServerError, err.Error()) @@ -575,16 +568,16 @@ func rerunJob(ctx *app_context.Context, job *actions_model.ActionRunJob, shouldB return nil } - job.TaskID = 0 - job.Status = actions_model.StatusWaiting + initialStatus := actions_model.StatusWaiting if shouldBlock { - job.Status = actions_model.StatusBlocked + initialStatus = actions_model.StatusBlocked + } + if err := job.PrepareNextAttempt(initialStatus); err != nil { + return err } - job.Started = 0 - job.Stopped = 0 if err := db.WithTx(ctx, func(ctx context.Context) error { - _, err := actions_service.UpdateRunJob(ctx, job, builder.Eq{"status": status}, "task_id", "status", "started", "stopped") + _, err := actions_service.UpdateRunJob(ctx, job, builder.Eq{"status": status}, "attempt", "task_id", "status", "started", "stopped") return err }); err != nil { return err diff --git a/routers/web/repo/actions/view_test.go b/routers/web/repo/actions/view_test.go index c636570c7e..06af165c5c 100644 --- a/routers/web/repo/actions/view_test.go +++ b/routers/web/repo/actions/view_test.go @@ -521,6 +521,9 @@ func TestActionsViewRedirectToLatestAttempt(t *testing.T) { } func TestActionsRerun(t *testing.T) { + defer unittest.OverrideFixtures("routers/web/repo/actions/TestActionsRerun")() + unittest.PrepareTestEnv(t) + tests := []struct { name string runIndex int64 @@ -530,20 +533,20 @@ func TestActionsRerun(t *testing.T) { }{ { name: "rerun all", - runIndex: 187, + runIndex: 138574, jobIndex: -1, - expectedURL: "https://try.gitea.io/user2/repo1/actions/runs/187/jobs/0/attempt/2", + expectedURL: "https://try.gitea.io/user2/repo1/actions/runs/138574/jobs/0/attempt/3", }, { name: "rerun job", - runIndex: 187, + runIndex: 138574, jobIndex: 2, - expectedURL: "https://try.gitea.io/user2/repo1/actions/runs/187/jobs/2/attempt/3", + expectedURL: "https://try.gitea.io/user2/repo1/actions/runs/138574/jobs/2/attempt/6", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ctx, resp := contexttest.MockContext(t, "user2/repo1/actions/runs/187/rerun") + ctx, resp := contexttest.MockContext(t, "user2/repo1/actions/runs/138574/rerun") contexttest.LoadUser(t, ctx, 2) contexttest.LoadRepo(t, ctx, 1) ctx.SetParams(":run", fmt.Sprintf("%d", tt.runIndex))