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/<pull request number>.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 <mfenniak@noreply.codeberg.org> Co-authored-by: Andreas Ahlenstorf <andreas@ahlenstorf.ch> Co-committed-by: Andreas Ahlenstorf <andreas@ahlenstorf.ch>
2026-03-20 17:23:09 +01:00 · 2026-03-20 17:23:09 +01:00 · 1127aca2d2
commit 1127aca2d2
parent a27f9a719e
9 changed files with 211 additions and 22 deletions
--- 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 {
--- 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)
--- 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)
+}
--- 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)
+}
--- 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

--- a/routers/web/repo/actions/TestActionsRerun/action_run.yml
+++ 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"}}'
--- a/routers/web/repo/actions/TestActionsRerun/action_run_job.yml
+++ 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
--- 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
--- 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))