From 374b3d9f4d4470e50fe33fee463a7d62d17ee1d7 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Tue, 20 Jan 2026 17:34:59 +0100 Subject: [PATCH] fix: remove infinite loop in UpdateRunJobWithoutNotification when run in transaction (#10945) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #10893 introduced a retry loop to manage concurrent updates when updating the state of `action_run` in the function `UpdateRunJobWithoutNotification`. However, when `UpdateRunJobWithoutNotification` is called from within a transaction, the retry loop continues to read the same data from the DB (due to repeatable read isolation) and loops infinitely. As #10893 was later identified to not be required to fix the target problem (https://code.forgejo.org/forgejo/runner/issues/1302), this PR reverts the change. The only retained change is that the error `ErrActionRunOutOfDate` is a constant rather than `errors.New("run has changed")`. ## 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 - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [x] Reverted the test added for 10893 after confirming that it is the cause of the problem. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - 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. - [x] 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. - [ ] 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/.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/10945 Reviewed-by: Gusted Reviewed-by: Ellen Εμιλία Άννα Zscheile Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- models/actions/run_job.go | 12 ++----- models/actions/run_job_test.go | 58 ---------------------------------- 2 files changed, 2 insertions(+), 68 deletions(-) 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) -}