Request to change PR target branch (merge-base) sometimes does not work

[opatut]

• Gitea version (or commit ref): 1.12.5
• Git version: 2.20.1
• Operating system: Debian 10 (buster) from official binary download
• Database (use [x]):
  • PostgreSQL
  • MySQL
  • MSSQL
  • SQLite
• Can you reproduce the bug at https://try.gitea.io:
  • Yes (provide example URL)
  • No, I tried but I have no idea how to reproduce the issue.

Description

Changing the base branch of a pull request through the UI sometimes just does nothing. I have no clue what PRs this works on and which not, so I have no reliable way of reproducing this. The POST request to /<repo>/pull/<num>/target_branch never produces a response. Verbose logging of everything (according to the guide from the issue template) is only this:

==> router.log <==
2020/11/04 14:25:02 Started POST /XXX/XXX/pull/XXX/target_branch for <IP>

==> gitea.log <==
2020/11/04 14:25:02 ...s/context/context.go:330:func1() [D] Session ID: XXX
2020/11/04 14:25:02 ...s/context/context.go:331:func1() [D] CSRF Token: XXXX

No response, no more logging.

Whether or not the request works depends on the PR. It never works for the same PR, but other PRs work.

[6543]

sounds bad, I have the feeling it is because of SQLite ... but without any logs & no way to reproduce it myselve I dont know how to find the issue

[zeripath]

This is one of the few times when [database] LOG_SQL=true could be helpful.

It would also be helpful to get some logs just before the POST in case the deadlock occurs just before that.

[opatut]

I have the feeling it is because of SQLite

Is there a way I can migrate safely to a different database backend? Then I could verify if that's the case.

This is one of the few times when [database] LOG_SQL=true could be helpful.

Pastebin

It looks to me (I've not touched gitea code but written webapps myself) that the query before is to update notifications, and the queries after what I already posted (Session ID + CSRF Token) are for rerendering the page. I do not see any queries to mutate the actual target branch.

It would also be helpful to get some logs just before the POST in case the deadlock occurs just before that.

While that is certainly possible, I think it is unlikely. The request fails reliably on the same PR every time, regardless of what I do before. For example, I can "copy as curl" the request from chrome/firefox devtools and run it all on its own, without prior requests, and it will still fail. I don't need to load the PR's page itself for it to fail.

FYI I have Drone CI installed and checks enabled on the repository. Disabling the webhook does not unbreak the PR, though maybe when I have it disabled when creating the PR, it might never break. Hard to debug since I can't disable that for all the team, all the time, on the repo that can reproduce the issue :/

---

I noticed something while looking at the code, trying to understand the steps involved in the update of the PR.

Here, the function checkAndUpdateStatus is referenced in a comment, and the code is obviously trying to reproduce a similar thing.

gitea/services/pull/pull.go

Lines 163 to 166 in dfa7291

	// This is the same as checkAndUpdateStatus in check service, but also updates base_branch
	if pr.Status == models.PullRequestStatusChecking {
	pr.Status = models.PullRequestStatusMergeable
	}

That function, checkAndUpdateStatus, is doing a check to a prQueue that is not present in the copied version in pull.go:

gitea/services/pull/check.go

Lines 59 to 62 in dfa7291

	has, err := prQueue.Has(strconv.FormatInt(pr.ID, 10))
	if err != nil {
	log.Error("Unable to check if the queue is waiting to reprocess pr.ID %d. Error: %v", pr.ID, err)
	}

Well, looking at both parts, and the git blame of those files, I notice that the comment, This is the same as checkAndUpdateStatus in check service, but also updates base_branch, is grossly outdated:

• The implementation in check.go received the check of the prQueue after a part of the function was copied to pull.go.
• The implementation in pull.go received logic to update divergence information between the status change and the DB update after the code was copied there.

Also, while tracing backwards the implementation of checkAndUpdateStatus, I found this commit 0f3923c from 2017, called "fix potential lock when sqlite". Seems to me like there has been a lot of effort making checkAndUpdateStatus work well, so why not use it?

Overall, I'm wondering if the copy of the implementation is really necessary or if it should be replaced with a call to the original function and either an additional argument for saving more information (base_branch), or with a subsequent UPDATE query to just update that one column afterwards.

[6543]

@opatut one thing we could test ... to change base-branch via api ... https://try.gitea.io/api/swagger#/repository/repoEditPullRequest

And #13381 start refactoring things ...

[opatut]

@opatut one thing we could test ... to change base-branch via api ... https://try.gitea.io/api/swagger#/repository/repoEditPullRequest

==> /var/lib/gitea/log/router.log <==
2020/11/09 14:06:34 Started PATCH /api/v1/repos/XXX/XXX/pulls/675?access_token=XXX for 82.194.XX.XX

Same problem. Request does not terminate, spinner goes indefinitely. If I use an invalid branch name it exits with an error though (e.g. {"base": "foo"} instead of {"base": "master"}).

[parnic-sks]

We are seeing the same thing on our Gitea 1.13.2 installation using PostgreSQL. Changing target branches on PRs never works for us. Anything else we can do to help diagnose the issue?

[zeripath]

I suspect that #14598 will fix this too.

[zeripath]

@parnic-sks would you be able to test now that #14598 has been merged?

[parnic-sks]

@parnic-sks would you be able to test now that #14598 has been merged?

@zeripath Confirmed; LGTM, thanks! Now I just want to look into adding a progress indicator of some kind while the XHR happens so the user knows the switch is pending. Hopefully I can get a PR up for that once I learn how to correctly rebuild the frontend/template/i18n files when they've changed. :)

[zeripath]

OK I'll close this and mark it as fixed by #14598