owen/gitocean
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix: remove infinite loop in UpdateRunJobWithoutNotification when run in transaction (#10945)

Browse Source 
#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/<pull request number>.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 <gusted@noreply.codeberg.org>
Reviewed-by: Ellen Εμιλία Άννα Zscheile <fogti@noreply.codeberg.org>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
This commit is contained in:
Mathieu Fenniak
2026-01-20 17:34:59 +01:00
committed by Mathieu Fenniak
parent c17d62569d
commit 374b3d9f4d
2 changed files with 2 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files
models/actions/run_job.go
+2 -10
View File
@@ -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
models/actions/run_job_test.go
-58
View File
@@ -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)
}