Merge script closing PRs instead of merging them randomly

[jakubgs]

For some time now we've been seeing the scripts/merge-pr.sh script we use to merge PRs randomly closing them, instead of marking them as merged. For example:

• nix: fortify the nix-update-gradle process #13369
• [#13331] Back button changes in chat view when scrolling (iOS) #13337
• Gradle: drop unavailable JCenter Bintray from config #13321

This appears to be some kind of timing issue, that is triggered when the PR branch is rebased and force pushed:
https://github.com/status-im/gh-rebase-fix-test/blob/150eedcafd346a46aa70fc3d13e90bcdf33e4c72/merge-pr.sh#L128-L132

As seen in the screenshot, it appears the PR is closed before the force-push takes effect. One major issue with this is that the commits and the diffs disappear from the PR, making it useless for documenting history of changes.

[jakubgs]

I have attempted to debug this issue by creating a new repository and trying to use the same script:
https://github.com/status-im/gh-rebase-fix-test

But despite trying several PRs I could not reproduce it:

• test 1 gh-rebase-fix-test#1
• test 2 gh-rebase-fix-test#2
• test 3 gh-rebase-fix-test#3
• test 4 gh-rebase-fix-test#4
• Feature e gh-rebase-fix-test#5

It just does not manifest. Which is why I thought this might be related to repository size.

In all cases I've added at least one or more commits in the target branch to trigger a rebase on the PR branch and force push.

[jakubgs]

I have created a fork of status-react under my own account: https://github.com/jakubgs/status-react

And attempted several PRs as well:

• https://github.com/jakubgs/status-react/pull/1
• https://github.com/jakubgs/status-react/pull/2
• https://github.com/jakubgs/status-react/pull/3

And yet still, no cigar. Just cannot reproduce it.

[jakubgs]

I think at this point it's sensible to involve GitHub support in this.

[jakubgs]

Here's the support ticket: https://support.github.com/ticket/personal/0/1629892

[jakubgs]

Here is another weird case, but not exactly the same, in this PR it first showed up as closed but then switched to merged:

• google free and metrics free #13360

Except despite being marked as merged it still lost all commits and diffs:

[jakubgs]

We got a response from support:

Thanks for your patience here! We were able to reproduce the bug with the following steps:

1. Create a PR.
2. Amend the PR's head branch (locally)
3. Merge the PR's head branch (locally) into the base branch.
4. Push the base branch and the head branch.
5. Depending on the order in which you do step "4", you get different outcomes:

• If you push the base branch first, then head branch, the PR will be marked as closed.
• If you push the head branch first, then the base branch, the PR will be marked as merged.
• If you push both base and head branches in the same invocation of git push (ie. with --all or similar), then one of the above cases will apply, depending on the lexicographical sort order of the two ref names.

Ideally, the outcomes should be the same regardless of the push order i.e PR marked as merged. We have had a few reports of this bug and our engineers are already working fo fix. It's very useful for us to have more data points, so thank you for submitting this report!

But their description is wrong. They say that:

• If you push the base branch first, then head branch, the PR will be marked as closed.

But our case is clearly:

• If you push the head branch first, then the base branch, the PR will be marked as merged.

Except we get a PR becoming "closed". Which I pointed out in a response.

[jakubgs]

It looks like this might actually be fixed:

The good news is, our engineering team has already figured out why and how this is happening. I'll share this with them as an extra data point to consider while working on the fix.

There is hope @flexsurfer.

[jakubgs]

I poked them for an update and they said:

Thanks for your patience here. A fix for this issue has been released internally. Our engineering team is yet to review its risks an impact before making it public. I am unable to give you a definite timeline of when the fix would made available publicly but you can keep an eye on our official GitHub public product roadmap, our Changelog and our GitHub Blog for future updates.

I'll have to close out this ticket while continuing to track the engineering ticket internally, though that doesn't mean we've forgotten about it! We'll be back in touch when we have any updates!

So we might just get there eventually.

[flexsurfer]

interesting if it fixes only new PRs or old already broken as well

[cammellos]

Closing as this is not actionable on our side, moving to discussion