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

Renovate deleted a PR branch that contained additional non-bot commits #2391

Closed
edmorley opened this issue Aug 15, 2018 · 5 comments
Closed
Labels
priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:bug Bug fix of existing functionality

Comments

@edmorley
Copy link
Contributor

What Renovate type are you using?
Renovate GitHub App

Describe the bug
Renovate repeatedly deletes a renovate branch that was manually updated by a human.

See:
neutrinojs/neutrino#1002

To Reproduce
Steps to reproduce the behavior:

  1. Add an additional commit to an existing Renovate PR (meaning there are now two commits in the PR), but also rebase both commits onto latest master
  2. Accidentally close the PR
  3. Reopen the PR

Expected behavior
Renovate doesn't delete the branch / modify the contents, since a human had added a second commit.

@rarkins
Copy link
Collaborator

rarkins commented Aug 15, 2018

Looks like it was a "race condition":

{"msg":"processBranch()","time":"2018-08-15T16:37:04.521Z"}
{"msg":"branchExists(renovate/babel-merge-2.x)=true","time":"2018-08-15T16:37:04.521Z"}
{"msg":"branchExists=true","time":"2018-08-15T16:37:04.521Z"}
{"level":30,"repository":"mozilla-neutrino/neutrino-dev","branch":"renovate/babel-merge-2.x","dependencies":["babel-merge"],"msg":"Branch has 1 upgrade(s)","time":"2018-08-15T16:37:04.521Z"}
{"msg":"recreateClosed is false","time":"2018-08-15T16:37:04.521Z"}
{"msg":"findPr(renovate/babel-merge-2.x, Update dependency babel-merge to v2, closed)","time":"2018-08-15T16:37:04.521Z"}
{"msg":"Found PR #1002","time":"2018-08-15T16:37:04.521Z"}
{"msg":"Found closed PR with current title","time":"2018-08-15T16:37:04.521Z"}
{"level":30,"repository":"mozilla-neutrino/neutrino-dev","branch":"renovate/babel-merge-2.x","dependencies":["babel-merge"],"prTitle":"Update dependency babel-merge to v2","msg":"Closed PR already exists. Skipping branch.","time":"2018-08-15T16:37:04.521Z"}
{"msg":"Getting comments for #1002","time":"2018-08-15T16:37:04.521Z"}
{"msg":"GET repos/mozilla-neutrino/neutrino-dev/issues/1002/comments?per_page=100 [retries=5]","time":"2018-08-15T16:37:04.521Z"}
{"msg":"Found 4 comments","time":"2018-08-15T16:37:04.756Z"}
{"msg":"Ensuring comment \"Renovate Ignore Notification\" in #1002","time":"2018-08-15T16:37:04.756Z"}
{"msg":"POST repos/mozilla-neutrino/neutrino-dev/issues/1002/comments [retries=5]","time":"2018-08-15T16:37:04.756Z"}
{"level":30,"repository":"mozilla-neutrino/neutrino-dev","branch":"renovate/babel-merge-2.x","dependencies":["babel-merge"],"issueNo":1002,"msg":"Added comment","time":"2018-08-15T16:37:05.461Z"}
{"msg":"DELETE repos/mozilla-neutrino/neutrino-dev/git/refs/heads/renovate/babel-merge-2.x [retries=5]","time":"2018-08-15T16:37:05.462Z"}

Renovate started running right after the PR closure as it triggers a webhook. It sees that the branch exists but the PR is closed. Normally people want that branch deleted, so that's what happened. However in the meantime (after Renovate retrieved the PR list, before it took action) the PR was reopened so then by deleting the branch, it also re-closed the PR. It doesn't look like the extra commit or rebasing had anything to do with it, so the shorter reproduction steps would be:

  1. Close a Renovate PR
  2. Wait for Renovate to run again
  3. After Renovate starts running and has cached the PR list, reopen the PR

I'll need to think how to best avoid this type of race condition. We don't want to be re-fetching every single branch/PR/issue at all times just in case one has changed since we started, as it would greatly increase the API use and potentially exhaust it.

@edmorley
Copy link
Contributor Author

Thank you for looking into this.

The reason I thought the human commits would have made a difference - is that normally they result in the "this PR has been modified, Renovate won't update it any more" message. However even before the PR was closed, we hadn't received such a message.

In addition, perhaps Renovate should be checking when deleting branches, and only doing so if they didn't contain extra commits? (These extra commits should presumably still show up in the cached PR list, so not need additional API requests, yeah?)

@rarkins
Copy link
Collaborator

rarkins commented Aug 15, 2018

@edmorley actually the edited notification message was there from 9 days ago:
image

And I think in a case like this, people wouldn't want the branch deleted regardless of number of commits. i.e. if you close and then reopen a PR by accident, you don't want it then deleted by Renovate. So maybe the requirement is checking one more time (with a cache-busted request) that the PR is still closed before deleting the branch.

@edmorley
Copy link
Contributor Author

Ah sorry missed that bot message!

Yeah agree perhaps a cache-busted request is the only way. Though hopefully the "PR closed without merge" case is infrequent so not an issue with API request credits.

@rarkins rarkins added type:bug Bug fix of existing functionality priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others ready labels Aug 16, 2018
@rarkins rarkins removed the ready label Aug 16, 2018
@renovate-bot
Copy link
Collaborator

🎉 This issue has been resolved in version 13.46.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:bug Bug fix of existing functionality
Projects
None yet
Development

No branches or pull requests

3 participants