diff --git a/services/actions/rerun.go b/services/actions/rerun.go index 4ca32276d1..e2e7a819d4 100644 --- a/services/actions/rerun.go +++ b/services/actions/rerun.go @@ -13,6 +13,8 @@ import ( "forgejo.org/models/db" "forgejo.org/models/unit" "forgejo.org/modules/container" + "forgejo.org/modules/timeutil" + "forgejo.org/modules/util" "xorm.io/builder" ) @@ -153,16 +155,28 @@ func RerunJob(ctx context.Context, job *actions_model.ActionRunJob) ([]*actions_ } for _, jobToRerun := range GetAllRerunJobs(job, jobs) { + // If the dependent job is still running, cancel it so that it can be rerun, too. Its results are obsolete + // when the job it depends on is rerun. + if !jobToRerun.Status.IsDone() { + if err := cancelSingleJob(ctx, jobToRerun, actions_model.StatusCancelled); err != nil { + return fmt.Errorf("cannot cancel dependent job %d with status %s: %w", + jobToRerun.ID, jobToRerun.Status, err) + } + + // Refresh the job after cancellation. + if jobToRerun, err = actions_model.GetRunJobByID(ctx, jobToRerun.ID); err != nil { + return fmt.Errorf("cannot refresh cancelled dependent job %d: %w", jobToRerun.ID, err) + } + } + canBeRerun, err := jobToRerun.CanBeRerun(ctx) if err != nil { return fmt.Errorf("cannot determine whether job %d can be rerun: %w", jobToRerun.ID, err) } - // Skipping jobs that cannot be rerun is wrong. They should be cancelled and rerun, instead, because they - // are dependent jobs and the old results might be worthless, anyway. But we keep that behaviour for now, - // because changing it requires more rework. + // This should never happen because the run was validated and the job cancelled if it was running. if !canBeRerun { - continue + return fmt.Errorf("cannot rerun dependent job %d", jobToRerun.ID) } // The job that should be rerun cannot be blocked, even if it has needs. @@ -201,3 +215,35 @@ func rerunSingleJob(ctx context.Context, job *actions_model.ActionRunJob, initia return nil } + +// cancelSingleJob cancels the given job and its associated task, if any. outcomeStatus defines the status that should +// be assigned to the cancelled job and its associated task after cancellation; a non-terminal status will result in an +// error. Nothing happens if the job has already been completed. +func cancelSingleJob(ctx context.Context, job *actions_model.ActionRunJob, outcomeStatus actions_model.Status) error { + if !outcomeStatus.IsDone() { + return fmt.Errorf("outcomeStatus must be a terminal status, but is: %s", outcomeStatus) + } + if job.Status.IsDone() { + return nil + } + + return db.WithTx(ctx, func(ctx context.Context) error { + if job.TaskID == 0 { + job.Status = outcomeStatus + job.Stopped = timeutil.TimeStampNow() + _, err := UpdateRunJob(ctx, job, nil, "status", "stopped") + if err != nil { + return fmt.Errorf("could not cancel job %d: %w", job.ID, err) + } + } + + // A task might have been created while we're trying to cancel the job. Therefore, always try to stop the task. + if err := StopTask(ctx, job.TaskID, outcomeStatus); err != nil { + if errors.Is(err, util.ErrNotExist) { + return nil + } + return err + } + return nil + }) +} diff --git a/services/actions/rerun_test.go b/services/actions/rerun_test.go index fcd638e448..ad25846ccc 100644 --- a/services/actions/rerun_test.go +++ b/services/actions/rerun_test.go @@ -225,6 +225,58 @@ func TestRerun_RerunJob(t *testing.T) { assert.Equal(t, timeutil.TimeStamp(0), dependentJob.Stopped) }) + t.Run("Rerun job needed by others with cancellation", func(t *testing.T) { + defer unittest.OverrideFixtures("services/actions/TestRerun_RerunJob")() + require.NoError(t, unittest.PrepareTestDatabase()) + + job := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: 683911}) + + rerunJobs, err := RerunJob(t.Context(), job) + require.NoError(t, err) + + assert.Equal(t, int64(683911), rerunJobs[0].ID) + assert.Equal(t, int64(683912), rerunJobs[1].ID) + + // Cancel the first job so that we can rerun it. + require.NoError(t, cancelSingleJob(t.Context(), job, actions_model.StatusFailure)) + + job = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: 683911}) + + assert.Equal(t, int64(2), job.Attempt) + assert.Equal(t, actions_model.StatusFailure, job.Status) + assert.Equal(t, timeutil.TimeStamp(0), job.Started) + assert.NotZero(t, job.Stopped) + + dependentJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: 683912}) + + assert.Equal(t, int64(2), dependentJob.Attempt) + assert.Equal(t, actions_model.StatusBlocked, dependentJob.Status) + assert.Equal(t, timeutil.TimeStamp(0), dependentJob.Started) + assert.Equal(t, timeutil.TimeStamp(0), dependentJob.Stopped) + + // Rerun again. The dependent job should be cancelled first. + rerunJobs, err = RerunJob(t.Context(), job) + require.NoError(t, err) + + assert.Len(t, rerunJobs, 2) + assert.Equal(t, int64(683911), rerunJobs[0].ID) + assert.Equal(t, int64(683912), rerunJobs[1].ID) + + job = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: 683911}) + + assert.Equal(t, int64(3), job.Attempt) + assert.Equal(t, actions_model.StatusWaiting, job.Status) + assert.Equal(t, timeutil.TimeStamp(0), job.Started) + assert.Equal(t, timeutil.TimeStamp(0), job.Stopped) + + dependentJob = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: 683912}) + + assert.Equal(t, int64(3), dependentJob.Attempt) + assert.Equal(t, actions_model.StatusBlocked, dependentJob.Status) + assert.Equal(t, timeutil.TimeStamp(0), dependentJob.Started) + assert.Equal(t, timeutil.TimeStamp(0), dependentJob.Stopped) + }) + t.Run("Rerun job with needs", func(t *testing.T) { defer unittest.OverrideFixtures("services/actions/TestRerun_RerunJob")() require.NoError(t, unittest.PrepareTestDatabase()) diff --git a/services/actions/run.go b/services/actions/run.go index d0e874ff54..3b52a4070a 100644 --- a/services/actions/run.go +++ b/services/actions/run.go @@ -11,7 +11,6 @@ import ( actions_model "forgejo.org/models/actions" "forgejo.org/models/db" - "forgejo.org/modules/timeutil" ) func killRun(ctx context.Context, run *actions_model.ActionRun, newStatus actions_model.Status) error { @@ -21,20 +20,7 @@ func killRun(ctx context.Context, run *actions_model.ActionRun, newStatus action return err } for _, job := range jobs { - oldStatus := job.Status - if oldStatus.IsDone() { - continue - } - if job.TaskID == 0 { - job.Status = newStatus - job.Stopped = timeutil.TimeStampNow() - _, err := UpdateRunJob(ctx, job, nil, "status", "stopped") - if err != nil { - return err - } - continue - } - if err := StopTask(ctx, job.TaskID, newStatus); err != nil { + if err := cancelSingleJob(ctx, job, newStatus); err != nil { return err } }