forked from mirrors/forgejo
test: concurrent merges targeting separate branches proceed without errors (#9925)
Follow-up to #9868, #9916, #9922, adding an automated test that verifies no deadlocks occur when concurrently merging PRs in the same repo, with the same labels, and same milestone. Different base and head branches are used to ensure there are no git conflicts. Resolves #9785. ## 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... - [ ] in their respective `*_test.go` for unit tests. - [x] 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 - [x] I do not want this change to show in the release notes. - [ ] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9925 Reviewed-by: Gusted <gusted@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
Mathieu Fenniak
parent
a3c6c78e08
commit
40b06af048
1 changed files with 192 additions and 0 deletions
|
|
@ -5,15 +5,19 @@ package integration
|
|||
|
||||
import (
|
||||
"bytes"
|
||||
"encoding/base64"
|
||||
"fmt"
|
||||
"math/rand/v2"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"os"
|
||||
"path"
|
||||
"path/filepath"
|
||||
"slices"
|
||||
"strconv"
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
|
|
@ -30,6 +34,7 @@ import (
|
|||
"forgejo.org/modules/git"
|
||||
"forgejo.org/modules/gitrepo"
|
||||
"forgejo.org/modules/hostmatcher"
|
||||
"forgejo.org/modules/log"
|
||||
"forgejo.org/modules/queue"
|
||||
"forgejo.org/modules/setting"
|
||||
api "forgejo.org/modules/structs"
|
||||
|
|
@ -1185,3 +1190,190 @@ func TestRebaseWhenNecessary(t *testing.T) {
|
|||
})
|
||||
})
|
||||
}
|
||||
|
||||
func shuffleSlice(slice []int64) {
|
||||
rand.Shuffle(len(slice), func(i, j int) {
|
||||
slice[i], slice[j] = slice[j], slice[i]
|
||||
})
|
||||
}
|
||||
|
||||
func TestMergeConcurrency(t *testing.T) {
|
||||
onApplicationRun(t, func(t *testing.T, giteaURL *url.URL) {
|
||||
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
|
||||
token := getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteIssue)
|
||||
|
||||
// How many PRs will be created and attempted to be merge concurrently? 5 was enough to reproduce deadlock
|
||||
// errors from label and repo stat updates ~80% of the time, and it keeps the test reasonably fast. Test has
|
||||
// been run up to 50 without errors.
|
||||
concurrentCount := 5
|
||||
|
||||
repo, _, deferrer := tests.CreateDeclarativeRepo(t, user2, "concurrency-test", nil, nil, nil)
|
||||
defer deferrer()
|
||||
|
||||
// Create labels for the PRs...
|
||||
labelNames := []string{"bug", "super-important", "tricky", "deadlock"}
|
||||
labelIDs := make([]int64, 0, len(labelNames))
|
||||
for _, label := range labelNames {
|
||||
req := NewRequestWithJSON(t,
|
||||
"POST",
|
||||
fmt.Sprintf("/api/v1/repos/%s/%s/labels", repo.OwnerName, repo.Name),
|
||||
&api.CreateLabelOption{
|
||||
Color: "#000000",
|
||||
Name: label,
|
||||
}).AddTokenAuth(token)
|
||||
resp := MakeRequest(t, req, http.StatusCreated)
|
||||
|
||||
var apiLabel api.Label
|
||||
DecodeJSON(t, resp, &apiLabel)
|
||||
labelIDs = append(labelIDs, apiLabel.ID)
|
||||
}
|
||||
|
||||
// Create a milestone that all PRs will be associated to...
|
||||
req := NewRequestWithJSON(t,
|
||||
"POST",
|
||||
fmt.Sprintf("/api/v1/repos/%s/%s/milestones", repo.OwnerName, repo.Name),
|
||||
&api.CreateMilestoneOption{
|
||||
Title: "Test Milestone",
|
||||
Description: "This is the test milestone.",
|
||||
State: "open",
|
||||
}).AddTokenAuth(token)
|
||||
resp := MakeRequest(t, req, http.StatusCreated)
|
||||
var apiMilestone api.Milestone
|
||||
DecodeJSON(t, resp, &apiMilestone)
|
||||
|
||||
{
|
||||
var createAllPRs sync.WaitGroup
|
||||
var errorListMutex sync.Mutex
|
||||
var errorList []any
|
||||
for i := range concurrentCount {
|
||||
createAllPRs.Add(1)
|
||||
go func(i int) {
|
||||
defer createAllPRs.Done()
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
errorListMutex.Lock()
|
||||
defer errorListMutex.Unlock()
|
||||
errorList = append(errorList, r)
|
||||
}
|
||||
}()
|
||||
|
||||
// We're going to create two branches; a new target branch where the PR will merge *into*, and a new
|
||||
// head branch where the PR will merge *from*. This test is about finding internal concurrency
|
||||
// conflicts within Forgejo that prevent merges, and, merging simultaneously into the *same branch*
|
||||
// would have natural conflicts that aren't what we're attempting to test.
|
||||
targetBranchName := fmt.Sprintf("target-branch-%d", i)
|
||||
req := NewRequestWithJSON(t,
|
||||
"POST",
|
||||
fmt.Sprintf("/api/v1/repos/%s/%s/branches", repo.OwnerName, repo.Name),
|
||||
&api.CreateBranchRepoOption{
|
||||
OldRefName: "main",
|
||||
BranchName: targetBranchName,
|
||||
}).AddTokenAuth(token)
|
||||
MakeRequest(t, req, http.StatusCreated)
|
||||
|
||||
// Create the head branch that we'll be trying to merge from, with a file change:
|
||||
headBranchName := fmt.Sprintf("update-%d", i)
|
||||
req = NewRequestWithJSON(t,
|
||||
"POST",
|
||||
fmt.Sprintf("/api/v1/repos/%s/%s/contents/README-%d.md", repo.OwnerName, repo.Name, i),
|
||||
&api.CreateFileOptions{
|
||||
FileOptions: api.FileOptions{
|
||||
NewBranchName: headBranchName,
|
||||
},
|
||||
ContentBase64: base64.StdEncoding.EncodeToString(fmt.Appendf(nil, "Hello, world %d!\n", i)),
|
||||
}).AddTokenAuth(token)
|
||||
MakeRequest(t, req, http.StatusCreated)
|
||||
|
||||
// Create a PR for the branch
|
||||
myLabelIDs := slices.Clone(labelIDs)
|
||||
shuffleSlice(myLabelIDs) // use a random ordering for labels as it may cause deadlocks when their count of assigned issues is updated
|
||||
req = NewRequestWithJSON(t, http.MethodPost,
|
||||
fmt.Sprintf("/api/v1/repos/%s/%s/pulls", repo.OwnerName, repo.Name),
|
||||
&api.CreatePullRequestOption{
|
||||
Head: headBranchName,
|
||||
Base: targetBranchName,
|
||||
Title: fmt.Sprintf("create PR from branch %s", headBranchName),
|
||||
Labels: myLabelIDs,
|
||||
Milestone: apiMilestone.ID,
|
||||
}).AddTokenAuth(token)
|
||||
MakeRequest(t, req, http.StatusCreated)
|
||||
}(i)
|
||||
}
|
||||
createAllPRs.Wait()
|
||||
assert.Empty(t, errorList)
|
||||
}
|
||||
|
||||
// All our PRs are created; now let's try to merge them concurrently.
|
||||
|
||||
lc, logCleanup := test.NewLogChecker(log.DEFAULT, log.ERROR)
|
||||
lc.Filter(
|
||||
// Typical error when the test fails:
|
||||
"Failed to update PR to merged",
|
||||
|
||||
// MySQL & PostgreSQL will both report 40001 for deadlocks
|
||||
"40001",
|
||||
)
|
||||
lc.StopMark("===End Test: TestMergeConcurrency===")
|
||||
defer logCleanup()
|
||||
|
||||
{
|
||||
var mergeAllPRs sync.WaitGroup
|
||||
var errorListMutex sync.Mutex
|
||||
var errorList []any
|
||||
for i := range concurrentCount {
|
||||
mergeAllPRs.Add(1)
|
||||
go func(i int) {
|
||||
defer mergeAllPRs.Done()
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
errorListMutex.Lock()
|
||||
defer errorListMutex.Unlock()
|
||||
errorList = append(errorList, r)
|
||||
}
|
||||
}()
|
||||
|
||||
targetBranchName := fmt.Sprintf("target-branch-%d", i)
|
||||
headBranchName := fmt.Sprintf("update-%d", i)
|
||||
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{
|
||||
HeadRepoID: repo.ID,
|
||||
BaseRepoID: repo.ID,
|
||||
HeadBranch: headBranchName,
|
||||
BaseBranch: targetBranchName,
|
||||
})
|
||||
|
||||
req := NewRequestWithJSON(t,
|
||||
"POST",
|
||||
fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", repo.OwnerName, repo.Name, pr.Index),
|
||||
&forms.MergePullRequestForm{
|
||||
Do: "merge",
|
||||
MergeMessageField: fmt.Sprintf("Merge %s", targetBranchName),
|
||||
}).AddTokenAuth(token)
|
||||
resp := MakeRequest(t, req, http.StatusOK)
|
||||
assert.Empty(t, resp.Body.String())
|
||||
}(i)
|
||||
}
|
||||
mergeAllPRs.Wait()
|
||||
assert.Empty(t, errorList)
|
||||
}
|
||||
|
||||
// Verify that: Logs had no errors
|
||||
log.Error("===End Test: TestMergeConcurrency===")
|
||||
filtered, stopped := lc.Check(100 * time.Millisecond)
|
||||
assert.False(t, filtered[0], "received 'Failed to update PR to merged' error log messages")
|
||||
assert.False(t, filtered[1], "40001 deadlock errors were logged during test")
|
||||
assert.True(t, stopped)
|
||||
|
||||
// Verify that: All PRs are merged on the repo
|
||||
for i := range concurrentCount {
|
||||
targetBranchName := fmt.Sprintf("target-branch-%d", i)
|
||||
headBranchName := fmt.Sprintf("update-%d", i)
|
||||
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{
|
||||
HeadRepoID: repo.ID,
|
||||
BaseRepoID: repo.ID,
|
||||
HeadBranch: headBranchName,
|
||||
BaseBranch: targetBranchName,
|
||||
})
|
||||
assert.True(t, pr.HasMerged, "pr %s merged", headBranchName)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue