Lock file maintenance PR didn't use lockfile from master

[edmorley]

This is a:

• Bug report (non-security related)
• Feature request
• I'm not sure which of those it is

I'm using:

• The Renovate GitHub App
• Self-hosted GitHub
• Self-hosted GitLab
• Self-hosted VSTS

Please describe the issue:

Hi :-)

Renovate opened a lockfile update PR (neutrinojs/neutrino#743) which failed because of a problem on our side.

To resolve that, I created a new PR (neutrinojs/neutrino#744) that (a) removed the stray 2nd yarn.lock, (b) refreshed the lockfile manually to save Renovate from having to do so. I merged that PR, which also had a closes #NNN line in the commit message, causing the initial failing Renovate PR to be closed.

One minute after I merged my PR, Renovate opened a second lockfile update PR (presumably since it was still in the scheduled time window for refreshing, which itself is fine):
neutrinojs/neutrino#745

The problem is that the new PR duplicate the changes I made in my PR - implying that it wasn't rebased on master, even though my PR was merged before it opened. GitHub doesn't show the PR as having conflicts since the changes are identical (so presumably git auto-resolves them).

[rarkins]

Is it possible that the old branch was still there?

[edmorley]

Ah possibly, I don't remember deleting it (or more, I doubt I would have had time in the 30-60 seconds before the new PR opened),

[edmorley]

Presumably the lockfile update PR should always reset/delete any existing branch for lockfile refresh when creating a new PR?

[rarkins]

I may need to add a special rule for lock file PRs to avoid this edge case. You see, we deliberately don't keep updating/checking lock files for updates or we'd be running yarn non stop. Once a lock file maintenance PR has been created, we don't update it until you merge it or delete it. BUT I guess that may have this annoying side effect that if you close it without deleting it and Renovate runs again, it sees the existing branch and figures it doesn't need to run again. Then if there's no PR it thinks "I guess I should create that then". There's no logic to think "Looks like they closed the PR without deleting the branch - I'll start over and remove this branch first"

[edmorley]

Or perhaps another way of handling this might be for Renovate to delete a branch whenever anyone closes a PR? eg if they close a normal dependency update PR since they don't want the update, it presumably makes sense to delete that too?

[rarkins]

@edmorley Renovate actually does delete branches indirectly, but it's at the end of every "run". It looks at the list of branches that should be there (disregarding schedules) and then looks at the list of ones that are there, and it deletes any delta. The problem is that the lock file maintenance one should be there every time. I will think of a logic that can be added to try to detect this stale/closed case. It's made harder because of rules like prCreation=not-pending, because that means that simply seeing a branch without a PR does not mean that it was closed!

[edmorley]

Ah very true! The intersection of features makes things interesting :-)

[ghost]

I literally just read this 5 seconds before the person behind me saw the exact same behavior on their project.

So yeah, I'm totally in favor of a change that helps avoid the confusing case of Renovate filing a new lockfile maintenance pull request because the branch wasn't manually cleaned up in time.

[rarkins]

I'll take another look at lock file maintenance as soon as possible to try to avoid the couple of annoying behaviours you've seen lately.

[rarkins]

I think the trick may be a special logic like this: If branch exists, but PR doesn't, we need to work out if that's because the PR was closed or the PR wasn't opened yet. Best way may be to check for a Closed PR that pointed to the sha of the branch - I think GitHub may expose that data. If so then delete and recreate.

[edmorley]

I'm possibly overlooking something that was mentioned before, but could the logic instead go something like:
"if opening a lockfile PR and the branch exists, always try to rebase branch on master, and if wasn't already rebased, then update push updated branch. then perform pr-pending check as usual (if enabled)"

ie: it's not really the deleting the branch that we care about, it's having a rebased branch. And I don't think a lockfile PR should ever be opened from a not-rebased branch.

[rarkins]

We can't keep updating it non-stop, as otherwise we'll be churning bot cpu non-stop regenerating the yarn.lock and churning the PR and CI perhaps 5-10x a day every time any dependency in the lock file changes compared to the last time.

But perhaps what we can do is keep rebasing it only while the PR is not open. If you set prCreation=not-pending this could result in it also churning a few times if you're unlucky, but eventually there should be an hour with no lock file updates where it will get pushed.

[ghost]

If branch exists, but PR doesn't, we need to work out if that's because the PR was closed or the PR wasn't opened yet.

I believe that would address the issue we're running into. A team updated the lockfile in master as part of another change and just didn't want the lockfile PR any longer. They aren't expecting another lockfile PR until the regularly scheduled time (which is this upcoming Monday).

And I don't think a lockfile PR should ever be opened from a not-rebased branch.

So, many of our teams set the Require branches to be up to date before merging option to ensure all changes are building on master, and, in the case of changes to the project's lockfile, that the lockfile has been rebased on any changes in the master branch.

I do feel it's probably important that a lockfile PR always be re-generated after a rebase to ensure there aren't discrepancies in the data of the lockfile. However, in reality, that should probably be caught as part of the project's CI tests.

[ghost]

Because our teams use Require branches to be up to date before merging along with the rebaseStalePrs option, I think Renovate should respect that setup and continuously rebase the lockfile maintenance PR whenever there's a new commit to master. We'd be willing to pay the CI costs, but I don't know what this means for Renovate.

The goal, from our standpoint, is to avoid needing developers to press the Update branch button in the GitHub UI to rebase the PR branch. It's a step that can, and should, be automated. Furthermore, once the button has been pressed, developers have to wait another X number of minutes until CI passes again before they can accept the PR. That's time those developers aren't likely to start deep work elsewhere.

[ghost]

It should be noted that I don't need the PR branch re-generated, just rebased.

The lockfile should only be re-generated if we feel a rebase may introduce discrepancies in the lockfile.

[ghost]

And I don't think a lockfile PR should ever be opened from a not-rebased branch.

@edmorley do you feel there's a case for never accepting a non-rebased lockfile PR?

[rarkins]

Maybe it's time to rethink how this is being done.

• You usually have lock file maintenance scheduled e.g. weekly because you don't want a non-stop rain of "something in your lock file changed" PRs all week

• Easiest case: no existing PR exists, so Renovate creates one

• Default behaviour is that Renovate won't keep updating it non-stop in subsequent runs as the updates would be frequent, your CI would run regularly, and you may find it frustratingly long to get a "green" status before it changes again

Now what can happen after this?

1. If you merge it and delete the branch, all is good.
2. If you merge it and don't delete the branch, you would expect to see the branch removed by Renovate.
3. If you just close the PR and delete the branch, all is good.
4. If you close the PR but leave the branch, you would expect Renovate to remove it.
5. If the branch becomes conflicted before you can merge (e.g. due to merging other PRs that modify lockfile), you expect Renovate to recreate the branch to resolve conflicts
6. If the branch simply becomes stale (out of date with master) then maybe you want it rebsaed, maybe you don't (I think this is up to preference and config)

Challenges with the above:

• It can be hard for Renovate to tell the difference between prCreation=not-pending vs (4) above. e.g. if Renovate runs and finds a branch existing but without PR, we can't immediately assume the branch should be cleaned up. Also maybe it's set for branch-* automerge.

• Finding the balance between refreshing/revalidating the PR vs non-stop churn is tough. But maybe defaulting lockFileMaintenance to rebaseStalePrs=true is an OK tradeoff that solves most problems

@destroyerofbuilds @edmorley can you give your feedback?

[ghost]

You usually have lock file maintenance scheduled e.g. weekly because you don't want a non-stop rain of "something in your lock file changed" PRs all week

Correct.

On a related note, we have regularly scheduled releases of our applications (We don't release our application on every change). That's another reason why it's not valuable to update a lockfile multiple times between each regularly scheduled application release. Updating a dependency twice within our lockfile will be pointless since the first update will never be released into production.

Default behaviour is that Renovate won't keep updating it non-stop in subsequent runs as the updates would be frequent

I agree with the default behavior. I do not want the lockfile being re-generated for the sole purpose of getting updates.

---

1. Yep.
2. Not a hard requirement, but it would be nice if Renovate cleaned up after itself.
3. Yep.
4. I would expect Renovate to not re-open the pull request. Not a hard requirement, but it would be nice if Renovate cleaned up after itself.
5. Correct.
6. I would want Renovate to rebase so that developers do not need to press the Update branch button.

we can't immediately assume the branch should be cleaned up

Do branches and PRs have creation timestamps? Can those be used to determine if a PR was opened after the branch was created?

[rarkins]

I have fixed (6) above. If the branch needs rebasing according to usual rules (i.e. it's conflicted, or branch protection settings say it must be kept up-to-date, or rebaseStalePrs is set to true) then Renovate will now do so. I think this fixes the main problem here, but I'll only close once I'm sure there's no more changes to make.

[renovate-bot]

🎉 This issue has been resolved in version 11.36.10 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[edmorley]

Hi! Neither of the two projects where I have weekly Monday lockfile maintenance enabled got a PR opened today, and there isn't a branch created/pending. Did this perhaps regress something?

[rarkins]

@edmorley you're right, and sorry for that. Unfortunately the new pruning logic for "pointless" lock files was too aggressive and now fixed.

[edmorley]

That was fast! Many thanks :-)