Fix incorrect action duration time when rerun the job before executed once

[yp05327]

Fix #28323
Reason was mentioned here: #28323 (comment)

Changes: (maybe breaking)

We can rerun jobs in Gitea, so there will be some problems in calculating duration time.
In this PR, I use the exist Started and Stopped column to record the last run time instead of the total time,
and add a new PreviousDuration column to record the previous duration time.
You can also check the cost time of last run:

[wolfogre]

Thanks for figuring out the bug, but it may not be a good idea to always reset the Started and Stopped time of a run.

Users may rerun a specific job, not all jobs of a run.

What about

diff --git a/models/actions/run_job.go b/models/actions/run_job.go
index 4b8664077d..a96ab6a7c1 100644
--- a/models/actions/run_job.go
+++ b/models/actions/run_job.go
@@ -138,11 +138,20 @@ func UpdateRunJob(ctx context.Context, job *ActionRunJob, cond builder.Cond, col
                        return 0, err
                }
                run.Status = aggregateJobStatus(jobs)
-               if run.Started.IsZero() && run.Status.IsRunning() {
-                       run.Started = timeutil.TimeStampNow()
+               var started, stopped timeutil.TimeStamp
+               for _, job := range jobs {
+                       if !job.Started.IsZero() && (started.IsZero() || started > job.Started) {
+                               started = job.Started
+                       }
+                       if !job.Stopped.IsZero() && (stopped.IsZero() || stopped < job.Stopped) {
+                               stopped = job.Stopped
+                       }
                }
-               if run.Stopped.IsZero() && run.Status.IsDone() {
-                       run.Stopped = timeutil.TimeStampNow()
+               run.Started = started
+               if run.Status.IsDone() {
+                       run.Stopped = stopped
+               } else {
+                       run.Stopped = 0 // reset the stopped time, it could have been stopped before but is being rerun again.
                }
                if err := UpdateRun(ctx, run, "status", "started", "stopped"); err != nil {
                        return 0, fmt.Errorf("update run %d: %w", run.ID, err)

It should work but I haven't tested it.

[yp05327]

You are right. But it seems that the suggestion doesn't right.
The duration time will become bigger and bigger, as the start time may not be changed, but the stop time will be updated every time.

@wolfogre
Maybe the correct way is calculating the total duration of all jobs, but not simply calculate stop - start
We can hold Started for dynamically displaying the duration time when the job is running or before running.
But when it finished, we calculate the the total duration of all jobs, and save it into database.

[yp05327]

Convert Stopped into Duration for not only ActionRun but also ActionRunJob
ActionRunJob.Duration can be calculated by SUM(ActionTaskStep.Stopped - ActionTaskStep.Started)
ActionRun.Duration can be calculated by SUM(ActionRunJob.Duration)
This can avoid similar issue (maybe occurred?) in action run job too.

[wolfogre]

The duration time will become bigger and bigger, as the start time may not be changed, but the stop time will be updated every time.

It could happen, such as the run contains two jobs, the first one started and stopped yesterday and you rerun another job today. Then the duration of the run will be very long.

Maybe the correct way is calculating the total duration of all jobs, but not simply calculate stop - start

I'm not sure. The point is, how do you define the duration of a run? Jobs in a run could execute concurrently. Think about this: the run contains 100 jobs, all of them started at 9:00 and stopped at 9:01. So, would you say the run lasted for 1 minute or 100 minutes?

I prefer 1 minute because I just want to know how much time I need to wait, instead of how much work time the runner takes. Let's say it's pretty clear but "rerun some or all jobs of a run" makes it complex. So I know that my patch is not perfect, but it's good enough for me.

ActionRunJob.Duration can be calculated by SUM(ActionTaskStep.Stopped - ActionTaskStep.Started)

It couldn't be right while Gitea now treats some works before any step as Set up job, it's actually not a step, and so is Complete job.

This can avoid similar issue (maybe occurred?) in action run job too.

I don't think the issue exists. Steps execute sequentially, and the job will be completely reset when you rerun it. While you cannot completely reset a run when rerun some jobs of it. Gitea doestn't support rerun a run yet, it's just "rerun all jobs of it".

[yp05327]

Ah, I'm considering this problem under the premise that runner can only run one task at once. 😕

[yp05327]

how do you define the duration of a run?

I prefer the time cost in running instead of stopped time - started time here.

For example, the run contains 100 jobs, all of them started at 9:00 and stopped at 9:01 today. And I rerun one of them at 9:00 today, and it stopped at 9:01.
I prefer displaying 1minite (yesterday) + 1minite (today), but not 24h1m.

In another way, as a quick fix, maybe your solution is good enough.
But for the feature, we need to redesign the action, and rerun job should create new jobs but not overwrite it in frontend,
as user can not check the execution result before the rerun.

[yp05327]

@wolfogre Cc @lunny
I have changed the solution and updated the description of this PR.
If this solution looks good to you, I will add a migration later,
as the migration for this PR is simple, so we can quickly add it, but maintaining it is terrible.

[wolfogre]

Sorry, I don't think it's a good idea to maintain TotalDuration and reset Started, and your patch doesn't work well in some cases.

Think about this:

Time	Event	Run	Job1	Jobs2
9:00	start	running, 9:00- , TotalDuration 0m	running, 9:00-	running, 9:00-
9:05	job1 is done	running, 9:00- , TotalDuration 0m	done, 9:00-9:05	running, 9:00-
9:10	rerun job1	running, 9:10- , TotalDuration 0m	running (rerun), 9:10-	running, 9:00-
9:15	all jobs are done	done, 9:10-9:15, TotalDuration 5m	done, 9:10-9:15	done, 9:00-9:15

So the run's duration is 5m while the job2's is 15m.

[yp05327]

Then we only reset start time when it is done?

[wolfogre]

Then we only reset start time when it is done?

It makes sense, and I didn't find any logical loopholes.

---

But I think it could be simpler if:

• Rename TotalDuration to PreviousDuration.
  • Duration() = calculateDuration(Started, Stopped) + PreviousDuration
  • Set PreviousDuration += calculateDuration(Started, Stopped) only before resetting Started and Stopped
  • Old data can keep zero PreviousDuration, you don't need to migrate it.
• Give up DisplayDuration().
  • It's a little confusing, why do we need it when we have Duration()? What's the difference to users?
  • Leave the old data with negitive there, I don't think it needs to be fixed. I believe users don't care old runs.

[yp05327]

This sounds better. Let's do it. 😄

Old data can keep zero PreviousDuration, you don't need to migrate it.

I agree. My previous plan is only creating the new column in the migration.
So I said this can be done later.

[yp05327]

Migration seems strange. v284.go is not used.

[lunny]

Migration seems strange. v284.go is not used.

#28827

[GiteaBot]

I was unable to create a backport for 1.21. @yp05327, please send one manually. 🍵

go run ./contrib/backport 28364
...  // fix git conflicts if any
go run ./contrib/backport --continue

[github-actions]

Automatically locked because of our CONTRIBUTING guidelines