Closed jglick closed 1 year ago
Great write-up. We observe this issue regularly as well. See ScribeMD/docker-cache#175 for example, which Dependabot opened immediately after ScribeMD/docker-cache#174 was merged because it had been previously prevented from doing so by the open-pull-requests-limit
.
This may be due to the same root cause as https://github.com/dependabot/dependabot-core/issues/4031.
Good find. I would go a step further and close this as a duplicate of #4031. They seem 100% identical to me.
Your issue is definitely the same but @jglick's symtoms seem different, since there's no open-pull-request-limit
involved, right? We could still close it as a duplicate and mention the different ways to trigger the problem (outdated PRs being opened), not sure what @jeffwidman thinks.
This seems to happen when one PR is merged at just about the same time (within a minute) of another PR being opened, so I suspect there is a race condition—an unfounded assumption in the control flow that the head of the base branch has not changed while running some computation.
I think the cause is fundamentally the same, but I don't have strong feelings about how the issues are tracked. The fact that open-pull-request-limit
is enabled by default is simply one mechanism by which the race condition is systematically produced, exacerbating the severity of the bug.
The analysis in https://github.com/dependabot/dependabot-core/issues/4163#issuecomment-908415614 is similar.
Yep yep, root cause seems the same.
We could still close it as a duplicate and mention the different ways to trigger the problem (outdated PRs being opened), not sure what @jeffwidman thinks.
I don't feel strongly on this, but I do lean toward closing as a dupe in this case. Mostly because we have so many issues that if they've got the same root cause, having fewer allows me to sorta track them a bit better in my head when new ones are filed or folks internally ask what's most pressing.
So closing as a dupe of #4031 .
I think these are two separate issues, #4031 describes Dependabot opening a PR that is out of date, but the diff for that outdated PR is correct (both the content and commit were created against the same parent, but that parent was out of date), whereas this issue talks about the diff being wrong, which I think comes from the fact that the sha used to generate Dependabot's diff did not match the sha used to create the commit for the given PR.
It's been really hard to reliably reproduce the issue, but I've pushed a change that might fix this 🤞 please let me know if you observe any changes to this behavior 🙇!
I will try to remember to post a comment if it does happen again, but unfortunately will surely forget to mention if some months go by and it does not (that I have seen). :grimacing: Thanks!
Here is an example of Dependabot firing automatically immediately upon a Dependabot pull request being merged (with open-pull-request-limit: 1
). It was not triggered by its schedule, but rather because there was now an available slot to open a pull request in. It failed to run with the following error message:
Dependabot cannot open any more pull requests
The open pull request limit has been exceeded. The current limit is: 1.
Dependabot will open new pull requests once you merge or close the already open pull requests. You can also update this limit in the config file.
[Learn more](https://docs.github.com/github/managing-security-vulnerabilities/troubleshooting-dependabot-errors)
This doesn't seem like the intended behavior, but it's a significant step forward relative to opening already outdated pull requests since it works on the next run.
Thanks for checking in!
but rather because there was now an available slot to open a pull request in.
Yeah it was actually never the intended behavior to immediately open a PR as soon as a slot is available, but that was a side-effect that crept into Dependabot from another feature we'd shipped.
I just released a change that should prevent this retry from happening in the first place, so hopefully you won't see this error'ed run show up anymore 🤞
Awesome @jurre 💪! So I'll take my chances and will close this. Of course if everyone hits this again, please open a separate ticket about it!
I observe this change working as intended! Thank you!
I just released a change that should prevent this retry from happening in the first place, so hopefully you won't see this error'ed run show up anymore crossed_fingers
Wait, it seems that the 'fix' was to prevent new PRs from being opened when an existing PR was closed/merged.
While that does make this issue obsolete it seems like a move in the wrong direction. Now, with a daily Dependabot run and a limit of one PR at a time, I might never have a fully up-to-date repo. It was easy to automate having dependabot rebase out of date merges so it seems like a big hammer.
That said, the reason I was using 1 PR at a time setting was to avoid wasted action cycles due to all open PRs being rebased every time 1 got submitted. If that issue were fixed then I would just set a high limit and it'd be fine.
The default of 5 is probably too low given this new logic.
@melink14, you can trigger Dependabot manually as often as desired by navigating to Insights > Dependency graph > Dependabot.
@Kurt-von-Laven Right, but the goal is for dependency updates to be automated. :)
I appreciate that one fewer button push is preferable, but it's not so different from pushing a button to merge the pull request.
That's a fair point. My dependency bot updates are auto merged so generally there's no interaction if something doesn't go wrong. That said, the manual flow might be the primary case.
The truth is that I was using this behavior to make a Dependabot merge queue and that should probably have it's own proper solution (and issues are open for that already) so not aside from the surprise, I'm okay with the outcome here.
Take a look at the commits in https://github.com/jenkinsci/bom/pull/1179, https://github.com/jenkinsci/bom/pull/1189, and https://github.com/jenkinsci/bom/pull/1190 and note the times the PRs were created and the times when they were merged. If you look at https://github.com/jenkinsci/bom/pull/1189/commits/5c373ff3c419195b189f8c58fa62d6c93cd9c392, the original commit from https://github.com/jenkinsci/bom/pull/1189, it updates
cloudbees-folder
, as advertised, but alsojunit
as in https://github.com/jenkinsci/bom/pull/1179. https://github.com/jenkinsci/bom/commit/1b046f2bff37512592f6a3238f41982abbf87a56, the original commit from https://github.com/jenkinsci/bom/pull/1190, updatedpipeline-utility-steps
as advertised but alsocloudbees-folder
as in https://github.com/jenkinsci/bom/pull/1189. If you look at the diff tab of the PR when it is initially created, you see two artifacts being bumped when only one should have been.It seems that Dependabot is creating commits with the right content for being merged into the base branch, but the wrong parent. For example https://github.com/jenkinsci/bom/commit/5c373ff3c419195b189f8c58fa62d6c93cd9c392 is parented to https://github.com/jenkinsci/bom/commit/68176693be69b917414330dcd74fe21f2c8e3acb, which would be fine except that it actually includes the change in https://github.com/jenkinsci/bom/commit/9d02ba94412443784019a139518e8697a8099795 which is not an ancestor. This seems to happen when one PR is merged at just about the same time (within a minute) of another PR being opened, so I suspect there is a race condition—an unfounded assumption in the control flow that the head of the base branch has not changed while running some computation.
Running
@dependabot rebase
(as for example in https://github.com/jenkinsci/bom/pull/1190#issuecomment-1147946298) fixes the issue and creates legitimate commits. It does not seem to be strictly necessary to do so, since the final squash-merge commit contains only the expected changes.This issue occurs fairly frequently in the
jenkinsci/bom
repository and I am pretty sure it can be seen in other Dependabot PRs in the @jenkinsci org.