Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gracefully handle when a push to `into_branch` results in a non-fast-forward error #859

Merged
merged 2 commits into from Jan 31, 2020

Conversation

@noizwaves
Copy link
Contributor

noizwaves commented Jan 27, 2020

This PR introduces a fix for the issue reported in #856.

Scenario

The into_branch changes, either from a manually merged PR or directly pushed to while a Bors batch is running.

Previously

Bors would attempt to push to the into_branch, receive an error from the GitHub API, and then crash:

{{:badmatch,
  {:error, :push, 422,
   "{\"message\":\"Update is not a fast forward\",\"documentation_url\":\"https://developer.github.com/v3/git/refs/#update-a-reference\"}"}},
 [
   {BorsNG.Worker.Batcher, :complete_batch, 3,
    [file: 'lib/worker/batcher.ex', line: 518]},
   {BorsNG.Worker.Batcher, :maybe_complete_batch, 1,
    [file: 'lib/worker/batcher.ex', line: 499]},
   {BorsNG.Worker.Batcher, :handle_cast, 2,
    [file: 'lib/worker/batcher.ex', line: 84]},
   {:gen_server, :try_dispatch, 4,
    [file: 'gen_server.erl', line: 637]},
   {:gen_server, :handle_msg, 6,
    [file: 'gen_server.erl', line: 711]},
   {:proc_lib, :init_p_do_apply, 3,
    [file: 'proc_lib.erl', line: 249]}
 ]}

New Behavior

The code checks to see if the push failed due to a non-fast-forward update error, and then retries the batch again (without a bisect). This seems safe and efficient to do, as the batch has already built successfully at this point.

These pictures demonstrate the new behavior. In this scenario, PR 107 and 108 were enqueued into bors and when the batch started to run, a third PR was directly merged:

Screenshot showing the history that experienced an update to into_branch

The comment history for an affected PR in the batch

The changes

Some of the code changes were a result of changing the signature of Batcher.complete_batch/3 to return new state. This was required to model the fact a Batch could take a new path and change to :error after being :ok.

The meaningful changes happen in Batcher.complete_batch/3 where we match on the push_success = false case.

The behavior of ServerMock was also enhanced to exhibit specific kinds of failures for the :push message.

…forward error.

- add ability to mock error behavior on ServerMock (`:push`)
@noizwaves noizwaves requested a review from notriddle Jan 27, 2020
@noizwaves

This comment has been minimized.

Copy link
Contributor Author

noizwaves commented Jan 28, 2020

@notriddle Thanks for the feedback! I've fixed the dialyzer errors. The fix doesn't look great, but I'm unsure how to make it look cleaner.

@noizwaves

This comment has been minimized.

Copy link
Contributor Author

noizwaves commented Jan 31, 2020

@notriddle hey, just wanted to bump this PR to see if it could be merged now. Thanks in advance!

Copy link
Member

notriddle left a comment

bors r+

@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Jan 31, 2020

Sure, let's merge this.

bors bot added a commit that referenced this pull request Jan 31, 2020
Merge #859
859: Gracefully handle when a push to `into_branch` results in a non-fast-forward error r=notriddle a=noizwaves

This PR introduces a fix for the issue reported in #856.

## Scenario
The `into_branch` changes, either from a manually merged PR or directly pushed to while a Bors batch is running.

## Previously
Bors would attempt to push to the `into_branch`, receive an error from the GitHub API, and then crash:

```
{{:badmatch,
  {:error, :push, 422,
   "{\"message\":\"Update is not a fast forward\",\"documentation_url\":\"https://developer.github.com/v3/git/refs/#update-a-reference\"}"}},
 [
   {BorsNG.Worker.Batcher, :complete_batch, 3,
    [file: 'lib/worker/batcher.ex', line: 518]},
   {BorsNG.Worker.Batcher, :maybe_complete_batch, 1,
    [file: 'lib/worker/batcher.ex', line: 499]},
   {BorsNG.Worker.Batcher, :handle_cast, 2,
    [file: 'lib/worker/batcher.ex', line: 84]},
   {:gen_server, :try_dispatch, 4,
    [file: 'gen_server.erl', line: 637]},
   {:gen_server, :handle_msg, 6,
    [file: 'gen_server.erl', line: 711]},
   {:proc_lib, :init_p_do_apply, 3,
    [file: 'proc_lib.erl', line: 249]}
 ]}
```

## New Behavior
The code checks to see if the push failed due to a non-fast-forward update error, and then retries the batch again (without a bisect). This seems safe and efficient to do, as the batch has already built successfully at this point.

These pictures demonstrate the new behavior. In this scenario, PR 107 and 108 were enqueued into bors and when the batch started to run, a third PR was directly merged:

<img width="437" alt="Screenshot showing the history that experienced an update to into_branch" src="https://user-images.githubusercontent.com/1007983/73217237-c8121100-4114-11ea-9524-137b769fdb76.png">

<img width="755" alt="The comment history for an affected PR in the batch" src="https://user-images.githubusercontent.com/1007983/73217248-ce07f200-4114-11ea-9ff4-c62c5d818ed7.png">


<Picture of comment history on PR>

## The changes
Some of the code changes were a result of changing the signature of `Batcher.complete_batch/3` to return new state. This was required to model the fact a Batch could take a new path and change to `:error` after being `:ok`.

The meaningful changes happen in `Batcher.complete_batch/3` where we match on the `push_success = false` case.

The behavior of `ServerMock` was also enhanced to exhibit specific kinds of failures for the `:push` message. 

Co-authored-by: Adam Neumann <adam.neumann@gusto.com>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jan 31, 2020

Build succeeded

@bors bors bot merged commit e28eea4 into bors-ng:master Jan 31, 2020
3 checks passed
3 checks passed
Community-TC (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
bors Build succeeded
Details
notriddle added a commit to bors-ng/bors-ng.github.io that referenced this pull request Feb 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.