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