Fork mode: Previously edited branch does not get overridden/deleted

[devversion]

How are you running Renovate?

Self-hosted

If you're self-hosting Renovate, tell us what version of Renovate you run.

32.119.0

Please select which platform you are using if self-hosting.

github.com

If you're self-hosting Renovate, tell us what version of the platform you run.

Github

Was this something which used to work for you, and then stopped?

I never saw this working

Describe the bug

With fork mode, renovate stops updating a group when a PR has been manually modified previously. e.g.

1. Renovate sends an update for a group called angular-deps
2. The PR starts to fail because we have size failures.
3. A maintainer pushes the pull request to fix up the size failures (this is possible since the PR has allowed maintainers to modify the fork branch)
4. The PR gets merged.
5. A few days/weeks later, still no update is sent because Renovate still has the old branch that was modified by someone other than the Renovate Git user.

This issue was visible in the angular/components repository, and here is the self-hosted runner entry: https://github.com/angular/dev-infra/runs/7415118482?check_suite_focus=true (look for Last commit author does not match git author email)

The PR modified in (3) was not immortal and the branch never got deleted upon merge: angular/components#25187 (this PR is an example -- Note that the branch was deleted manually just today after discovering this)

Note: It's unclear to me how this is supposed to work. Should the branch be deleted on merge? This is obviously more difficult when Renovate operates in a fork. Or should we just disable maintainer edits when forkMode: true?

Also super happy to look into contributing a fix if the expected behavior is clear and it's not a super large undertaking.

Relevant debug logs

Logs

DEBUG: getBranchPr(ng-renovate/angular-shared-dev-infra-code) (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: findPr(ng-renovate/angular-shared-dev-infra-code, undefined, open) (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: findPr(ng-renovate/angular-shared-dev-infra-code, undefined, closed) (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: branchExists=true (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: dependencyDashboardCheck=undefined (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: recreateClosed is false (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: findPr(ng-renovate/angular-shared-dev-infra-code, build: update angular shared dev-infra code to cc061c7, !open) (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: findPr(renovate/angular-shared-dev-infra-code, build: update angular shared dev-infra code to cc061c7, !open) (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: prAlreadyExisted=false (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: Checking if PR has been edited (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: Last commit author does not match git author email - branch has been modified (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) "branchName": "ng-renovate/angular-shared-dev-infra-code", "lastAuthor": "paulgschwendtner@gmail.com", "gitAuthorEmail": "angular-robot@google.com" DEBUG: findPr(ng-renovate/angular-shared-dev-infra-code, undefined, !open) (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: Found PR #25180 (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) DEBUG: Found old PR but the SHA is different (repository=angular/components, branch=ng-renovate/angular-shared-dev-infra-code) "oldPrNumber": 25180, "oldPrSha": "f5d8e2acdc03b25318d6c78e8c9a420071d0c3eb", "branchSha": "f21a665004c41f68cff5effb8ad2c7b1217e728f" DEBUG: getBranchPr(ng-renovate/angular-shared-dev-infra-code) (repository=angular/components) DEBUG: findPr(ng-renovate/angular-shared-dev-infra-code, undefined, open) (repository=angular/components) DEBUG: findPr(ng-renovate/angular-shared-dev-infra-code, undefined, closed) (repository=angular/components) DEBUG: Ensuring Dependency Dashboard (repository=angular/components) DEBUG: ensureIssue(Dependency Dashboard) (repository=angular/components) DEBUG: Issue is open and up to date - nothing to do (repository=angular/components)

Have you created a minimal reproduction repository?

No reproduction repository

[rarkins]

For now I'd say that if you edit a PR and then merge it then you need to delete the source branch.

For future, could Renovate reliably know when it can delete such a branch? Eg iterate through closed pull requests, check the branch it found against the head sha?

[devversion]

For now I'd say that if you edit a PR and then merge it then you need to delete the source branch.

yeah, it's difficult to communicate this to the whole team, that's why I was hoping we could just prevent maintainer edits for the renovate PRs, and instead team members would just clone the PR and fix it up separately (that's what I'm usually doing)

For future, could Renovate reliably know when it can delete such a branch? Eg iterate through closed pull requests, check the branch it found against the head sha?

Yeah, you mean, it would iterate through closed PRs and checks if the current edited SHA was HEAD a previous renovate PR? And if so, it would not mark the branch as edited but rather let it be re-used/overridden? That would seem reasonable to me.

Alternatively it might be a fair assumption/mental model to make: If there is no pending/open PR with the SHA from the edited branch, then the manual edits are no longer relevant and can be discarded. Usually when a Renovate-owned branch is modified, a PR is already open and once the PR is closed, the edits either landed or should be discarded. I like this a lot, but I'm not sure if I miss something here.

[rarkins]

I think we used to have that assumption (if a branch exists but no PR and someone has edited it then assume we are free to force push over the changes) but it caused problems with some edge case. I guess we need to decide which inconvenience is worse

[codyoss]

I have noticed this in the past month or so as well. I think it would be great if there was a flag to tell the bot if it could delete or force push to the branch if PRs for said dep upgrade have been merged. To workaround this today I have had to just change the branch naming that is used to get things unstuck. We for instance have some golden files that commonly need updating when new versions of deps get pulled in so this happens to us currently after ever renovate PR.

[rarkins]

I'm ok with the idea of adding intelligence to detect if an existing branch can be deleted (or at least reused), although I'm not sure how easy that will be to implement. Right now the logic is something like this:

• if branchExists
  • if branchIsModified then skip branch

We can hopefully add some logic here to say detect "if branch is actually left over from a previously merged PR", assuming that we can get a list of all previously merged PRs and their HEAD SHA.

[devversion]

I think we used to have that assumption (if a branch exists but no PR and someone has edited it then assume we are free to force push over the changes) but it caused problems with some edge case

I wonder what this edge-case was back then. To me, this philosophy seems like the most predictable and obvious one. There might be situations I don't think of here though -- hence my interest in the edge-case, if you remember 😄

[rarkins]

IIRC the edge case may have been with onboarding

[devversion]

Ah that makes sense. Potentially we could have a special case for the onboardingBranch. So that the rules are:

1. If the current branch is the onboardingBranch, do not delete it if there is an associated PR
2. For other branches, delete them if there is no associated pull request.