Page MenuHomePhabricator

libup committed and uploaded an upgrade twice for the same package on the same repo
Closed, ResolvedPublic

Description

First upgrade: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GlobalUserGroups/+/680432

Parent:     9ae3915f (build: Updating eslint-config-wikimedia to 0.19.0)
Author:     libraryupgrader <tools.libraryupgrader@tools.wmflabs.org>
AuthorDate: 2021-04-15 22:42:48 +0000
Commit:     libraryupgrader <tools.libraryupgrader@tools.wmflabs.org>
CommitDate: 2021-04-16 20:56:29 +0000

build: Updating eslint-config-wikimedia to 0.20.0

Change-Id: I255b5ad7fa91cc438178e3d74a98722ac7109074

Second upgrade: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GlobalUserGroups/+/680666

Parent:     9ae3915f (build: Updating eslint-config-wikimedia to 0.19.0)
Author:     libraryupgrader <tools.libraryupgrader@tools.wmflabs.org>
AuthorDate: 2021-04-16 20:58:20 +0000
Commit:     libraryupgrader <tools.libraryupgrader@tools.wmflabs.org>
CommitDate: 2021-04-17 09:47:35 +0000

build: Updating eslint-config-wikimedia to 0.20.0

Change-Id: Ifba6cc98cdbf26dbda300c635a1fba6de75342af

The second patch was created while the first patch set was not merged - both have same parent (merge was 15 Minutes later)

Not sure how to avoid this.
Maybe the next run of libup should not start before the commit queue is empty?

Event Timeline

In theory https://gerrit.wikimedia.org/r/plugins/gitiles/labs/libraryupgrader/+/refs/heads/master/libup/tasks.py#132 is supposed to stop this from happening, but it is affected by race conditions that I thought would never happen in practice, oops.

Change 680831 had a related patch set uploaded (by Legoktm; author: Legoktm):

[labs/libraryupgrader@master] Check that the patch is from the latest log entry after flood control

https://gerrit.wikimedia.org/r/680831

Change 680831 merged by jenkins-bot:

[labs/libraryupgrader@master] Check that the patch is from the latest log entry after flood control

https://gerrit.wikimedia.org/r/680831

For some reason I don't remember, checking whether an open patch exists is done when the patch is being created, rather than when we're trying to push it (the call to gerrit.query_changes in ng.py). I think once that's fixed all the potential causes for double pushes should be fixed.

I am not sure if that race condition is tracked by the moved code.

timeline

  • 2021-04-15 22:42:48 +0000 - The first patch set is created, the patch and the log is stored in the pusher queue
  • 2021-04-16 20:56:29 +0000 - The first patch set is visible in gerrit, the patch is still the latest at this moment
  • 2021-04-16 20:58:20 +0000 - The second patch set is created, the patch and the log is stored in the pusher queue
  • 2021-04-16 21:12:14 +0000 - The first patch set is merged
  • 2021-04-17 09:47:35 +0000 - The second patch set is visible in gerrit, the patch is also the latest at this moment

It just 2 minutes in this example. Or I am missing a point which happen with your fix?
If the two minutes in the other directory, than the check should work (second patch set created before push of the first patch set)

Reedy renamed this task from libup commit and upload upgrade twice for the same package on the same repo to libup committed and uploaded an upgrade twice for the same package on the same repo.Apr 20 2021, 12:02 AM

Yeah, xSavitar pinged me on IRC so I disabled pushing for now :( Will try to get it re-enabled tonight.

Change 685951 had a related patch set uploaded (by Legoktm; author: Legoktm):

[labs/libraryupgrader@master] Hopefully avoid duplicate patches

https://gerrit.wikimedia.org/r/685951

Change 685951 merged by jenkins-bot:

[labs/libraryupgrader@master] Hopefully avoid duplicate patches

https://gerrit.wikimedia.org/r/685951

I tried to figure out how to introduce locks into the queue and eventually gave up. Instead I think we can avoid duplicate pushes by

  • Ensuring there are no open bump-dev-deps changes at push time
  • Ensuring the base commit is still the same, so no other commits have been merged into the repository since the patch was created.

I think both of those should prevent duplicate pushes, unless I'm overlooking something.

Umherirrender assigned this task to Legoktm.

Looks good for the me and should be a good guard for the current situation.