diff --git a/models/actions/run_job.go b/models/actions/run_job.go index 9ddae1e301..ddf622f9e9 100644 --- a/models/actions/run_job.go +++ b/models/actions/run_job.go @@ -5,14 +5,12 @@ package actions import ( "context" - "errors" "fmt" "slices" "time" "forgejo.org/models/db" "forgejo.org/modules/container" - "forgejo.org/modules/log" "forgejo.org/modules/timeutil" "forgejo.org/modules/util" @@ -176,7 +174,7 @@ func UpdateRunJobWithoutNotification(ctx context.Context, job *ActionRunJob, con } } - for { + { // Other goroutines may aggregate the status of the run and update it too. // So we need load the run and its jobs before updating the run. run, err := GetRunByID(ctx, job.RunID) @@ -204,16 +202,10 @@ func UpdateRunJobWithoutNotification(ctx context.Context, job *ActionRunJob, con } if updateRequired { // As the caller has to ensure the ActionRunNowDone notification is sent we can ignore doing so here. - if err := UpdateRunWithoutNotification(ctx, run, "status", "started", "stopped"); err != nil && errors.Is(err, ErrActionRunOutOfDate) { - // Retry update; another session affected `run` simultaneously. It wasn't necessarily another update - // from this same loop -- there are other codepaths that update `ActionRun`. - log.Debug("UpdateRunWithoutNotification failed with %v; looping for retry", err) - continue - } else if err != nil { + if err := UpdateRunWithoutNotification(ctx, run, "status", "started", "stopped"); err != nil { return 0, fmt.Errorf("update run %d: %w", run.ID, err) } } - break // exit retry loop } return affected, nil diff --git a/models/actions/run_job_test.go b/models/actions/run_job_test.go index caaaae2cf8..7c204d4eb9 100644 --- a/models/actions/run_job_test.go +++ b/models/actions/run_job_test.go @@ -8,7 +8,6 @@ import ( "forgejo.org/models/db" "forgejo.org/models/unittest" - "forgejo.org/modules/test" "code.forgejo.org/forgejo/runner/v12/act/jobparser" "github.com/stretchr/testify/assert" @@ -282,60 +281,3 @@ func TestActionRunJob_HasIncompleteWith(t *testing.T) { }) } } - -func TestUpdateRunJobWithoutNotificationConcurrency(t *testing.T) { - require.NoError(t, unittest.PrepareTestDatabase()) - - testJob := unittest.AssertExistsAndLoadBean(t, &ActionRunJob{ID: 192}) - testRun := unittest.AssertExistsAndLoadBean(t, &ActionRun{ID: testJob.RunID}) - - // UpdateRunJobWithoutNotification is intended to update the related `ActionRun`, setting its `Started`, `Stopped`, - // and `Status` field to an appropriate state considering the job update. It has a retry loop to perform this work - // even if `ActionRun` is updated concurrently. To test that loop, we're going to intercept the invocation of - // AggregateJobStatus and freeze that update process, perform a different modification to the run, and then release - // the frozen test. The retry loop should trigger and a second pass updating the `ActionRun` should succeed. - - syncBeginPoint := make(chan any) - syncMidPoint := make(chan any) - syncEndPoint := make(chan any) - firstPass := true - - defer test.MockVariableValue(&AggregateJobStatus, func(jobs []*ActionRunJob) Status { - // Synchronization here needs to handle the faact that `AggregateJobStatus` will be invoked twice -- pause - // correctly on the first run, but continue with no concerns on the second run. - if firstPass { - firstPass = false - // Signal that we're in AggregateJobStatus()... - close(syncBeginPoint) - // Wait until signalled to continue - <-syncMidPoint - } - return StatusCancelled - })() - - go func() { - testJob.Status = StatusCancelled - updated, err := UpdateRunJobWithoutNotification(t.Context(), testJob, nil, "status") - close(syncEndPoint) // close before asserts, so that the test doesn't hang if it fails - require.NoError(t, err) - assert.EqualValues(t, 1, updated) - }() - - // Wait until UpdateRunJobWithoutNotification reaches AggregateJobStatus()... - <-syncBeginPoint - - // Perform a concurrent modification to `ActionRun` - testRun.Status = StatusSkipped - err := UpdateRunWithoutNotification(t.Context(), testRun, "status") - require.NoError(t, err) - - // Signal for AggregateJobStatus to continue - close(syncMidPoint) - - // Wait for goroutine to complete - <-syncEndPoint - - // Reload the `ActionRun` - testRun = unittest.AssertExistsAndLoadBean(t, &ActionRun{ID: testJob.RunID}) - assert.Equal(t, StatusCancelled, testRun.Status) -}