futurelearn / squiddy

Github Actions bot
1 stars 1 forks source link

Fix race condition in bubble merge #27

Closed urbanautomaton closed 1 year ago

urbanautomaton commented 1 year ago

We've recently started seeing occasional weird merges with squiddy, where the PR branch itself is merged and its commits land in master, but github considers the PR "closed" rather than merged, and having zero commits. (e.g. https://github.com/futurelearn/futurelearn/pull/14124)

The bubble merge action performs the following steps:

None of these operations are directly on the PR, meaning that Github has to detect any relevant changes to the PR and update its state. This happens asynchronously, and is therefore prone to race conditions. I think the branch is getting merged before github notices the rebase, which it finds confusing (and who can blame it).

This PR tries to eliminate the possibility of race conditions by:

You can see this updated approach successfully waiting for the PR to pick up the rebase update here.

Screenshot 2023-03-20 at 17 18 05

I've also done some removal of dead code and a bit of test tidying in the preceding commits.

Feedback

Seem legit?

urbanautomaton commented 1 year ago

Yeah, possibly. This particular use case is actually addressed by merge queues, a more recent github feature that's unfortunately only available in private repositories for enterprise subscriptions.

So if we ever stump up for the posh seats (or github deign to add this feature to regular teams) I'll be very glad to delete this code. 😄

urbanautomaton commented 1 year ago

@squiddy-bot merge

urbanautomaton commented 1 year ago

Okay, this appears to be working, here's the wait catching a branch mismatch over on a PR merge in the futurelearn repo.