Race condition when closing tickets

122 views
Skip to first unread message

Clemens Heuberger

unread,
Apr 15, 2015, 1:58:50 AM4/15/15
to sage-...@googlegroups.com, Sara Kropf

Ticket #17221
http://trac.sagemath.org/ticket/17221#comment:20

#17221 had been set to positive_review before, then reopened, another change
(2e62790e) was pushed at
2015-04-13T04:22:22-07
and then set to positive_review again.

It was closed at
2015-04-14T12:44:07-07
with the branch field set to commit id 2e62790e;
however, 2e62790e is _not_ contained in 6.7.beta0, only the previous commit
2cd08fb8.

For this concrete ticket, the solution is probably to simply open a new ticket
with the lost commit.

What is the procedure to avoid such situations or to detect such situations, if
not by chance?

(I had a similar ticket a year ago, so it is not an isolated occurrence)

Regards,

CH

Volker Braun

unread,
Apr 15, 2015, 2:08:40 AM4/15/15
to sage-...@googlegroups.com, sara....@aau.at, clemens....@aau.at
The obvious answer: Don't change a ticket once it is positive review. Treat it like "closed" -- if you find a bug open a new ticket.

kcrisman

unread,
Apr 15, 2015, 11:05:40 AM4/15/15
to sage-...@googlegroups.com


The obvious answer: Don't change a ticket once it is positive review. Treat it like "closed" -- if you find a bug open a new ticket.


Hmm, but somehow I feel that "socially" that will not always work, unless such things automatically became blockers.  Positive review can often be wrong - and indeed you yourself will usually unset it to 'needs work' when the bots tell you there is a test failure, right?

Volker Braun

unread,
Apr 15, 2015, 11:16:34 AM4/15/15
to sage-...@googlegroups.com
Its of course fine for the release manager to set something back from positive review. Also, if there is an actual problem with the ticket then tests should fail (if they don't then thats a bug in itself), and you can also switch it back from positive review before I get to the ticket.

But you can't turn around and add another feature on a ticket that already has positive review.

Simon King

unread,
Apr 15, 2015, 11:44:04 AM4/15/15
to sage-...@googlegroups.com
Hi!

On 2015-04-15, Volker Braun <vbrau...@gmail.com> wrote:
> Its of course fine for the release manager to set something back from
> positive review.

Quod licet Iovi, non licet bovi...

> Also, if there is an actual problem with the ticket then
> tests should fail

Why? The reviewer could notice (too late) that the code introduces a
problem for which there is no test, yet.

> But you can't turn around and add another feature on a ticket that already
> has positive review.

Can you give a compelling reason why not? I mean, when the ticket branch
hasn't been merged into the develop branch?

Of course, there is a race condition: The scenario is that a reviewer
attempts to set the review back from "positive" to "needs work", and at
the same time the release manager attempts to merge the ticket branch into
the develop branch.

It should be possible to write a trac plugin (sorry, I can't do it, but
I am sure you can) such that
* the release manager's merge attempt implies that all attempts to change
the status of the ticket are blocked (with a nice message telling to open
a new ticket), and
* the release manager's merge attempt will automatically be aborted, if the
reviewer sets it back to "needs work" a second before the release manager
pushed the button.

Best regards,
Simon


Volker Braun

unread,
Apr 15, 2015, 12:05:27 PM4/15/15
to sage-...@googlegroups.com
On Wednesday, April 15, 2015 at 5:44:04 PM UTC+2, Simon King wrote:
> Its of course fine for the release manager to set something back from
> positive review.
Quod licet Iovi, non licet bovi...

Tautologically true as long as we are talking about a race specifically about Jovi not noticing something in time.

> But you can't turn around and add another feature on a ticket that already
> has positive review.
Can you give a compelling reason why not? I mean, when the ticket branch
hasn't been merged into the develop branch? 

Testing imposes a significant delay between merging into the develop branch and closing the ticket. Sure we can have a more complicated trac workflow, but one way or another it'll just boil down to not changing positively-reviewed tickets unless the release manager finds an issue. 

kcrisman

unread,
Apr 15, 2015, 1:36:45 PM4/15/15
to sage-...@googlegroups.com

> But you can't turn around and add another feature on a ticket that already
> has positive review.

Not another feature, sure.  But fixing an error, even a typo (especially if it's a misleading typo like forgetting the word "not") sounds like adequate grounds to revert a positive review.  Our testing process should accommodate quality, not vice versa.
 
Testing imposes a significant delay between merging into the develop branch and closing the ticket.

My understanding of git is of course weaker than anyone else's, but ... isn't the whole POINT of having a DVCS so that one can test a change and do whatever one wants before actually merging into the develop branch?  Again, I would argue that it is far less likely for someone to respond to even a fairly major bug in a "new" ticket than to do a quick fix so that their contribution "gets in" with a ticket that is so so so close to being merged.

Volker Braun

unread,
Apr 15, 2015, 1:59:17 PM4/15/15
to sage-...@googlegroups.com
Either way, there needs to be a point where you can't go back and change the branch and expect it to be in the next release.

Simon King

unread,
Apr 15, 2015, 4:58:38 PM4/15/15
to sage-...@googlegroups.com
Hi Volker,

On 2015-04-15, Volker Braun <vbrau...@gmail.com> wrote:
>> Can you give a compelling reason why not? I mean, when the ticket branch
>> hasn't been merged into the develop branch?
>
>
> Testing imposes a significant delay between merging into the develop branch
> and closing the ticket.

Aha, you mean the problem is that the process of merging it into the
develop branch takes a significant time, so that it is easily possible
that the reviewer thinks "well, it hasn't been merged yet, so, we can
still work on it"?
OK, that's a good reason to discourage withdrawing positive reviews ---
unless we have a way to freeze a ticket while the release manager is
merging.

> Sure we can have a more complicated trac workflow,
> but one way or another it'll just boil down to not changing
> positively-reviewed tickets unless the release manager finds an issue.

Would it be really so complicated? I suppose you have a script that
merges into develop and then tests the branches of positively reviewed
ticket (or are you doing it manually?). That script could also contain a
line that blocks the associated ticket (i.e., makes it impossible for a
non-administrator to change the ticket status) while the script is
running.

Best regards,
SImon

Daniel Krenn

unread,
Apr 15, 2015, 5:28:37 PM4/15/15
to sage-...@googlegroups.com
Am 2015-04-15 um 22:58 schrieb Simon King:
> Would it be really so complicated? I suppose you have a script that
> merges into develop and then tests the branches of positively reviewed
> ticket (or are you doing it manually?). That script could also contain a
> line that blocks the associated ticket (i.e., makes it impossible for a
> non-administrator to change the ticket status) while the script is
> running.

If there is a script for closing tickets, it could be extended to check
if the status is still positive_review and if the current branch on the
ticket was already (really) merged into develop...

Best wishes,

Daniel

Volker Braun

unread,
Apr 15, 2015, 5:37:02 PM4/15/15
to sage-...@googlegroups.com
On Wednesday, April 15, 2015 at 11:28:37 PM UTC+2, Daniel Krenn wrote:
If there is a script for closing tickets, it could be extended to check
if the status is still positive_review and if the current branch on the
ticket was already (really) merged into develop...


Even then, the correct workflow is to not change tickets after setting them to positive review. Having the release manager chase after branches as people keep adding stuff after review is not a sustainable workflow. 

Nils Bruin

unread,
Apr 15, 2015, 7:33:51 PM4/15/15
to sage-...@googlegroups.com
On Wednesday, April 15, 2015 at 2:37:02 PM UTC-7, Volker Braun wrote:
Even then, the correct workflow is to not change tickets after setting them to positive review. Having the release manager chase after branches as people keep adding stuff after review is not a sustainable workflow. 

I completely concur.  The whole meaning of "positive review" goes out of the window if one still changes the branch on the ticket. Obviously, before someone can change the branch on the ticket, they should set its status to "needs work" (or at least some other status. Perhaps the act of changing the branch should automatically remove "positive review" status? However, even with doing that, there is still a race condition possible. I'm not quite sure how likely this one is and hence whether we need to protect against it. The consequences are just an unmerged further change, which should not be a huge disaster:

0. Ticket is at "positive review"
1. Release Manager (RM) starts testing/merging process
2. Ticket gets set to "needs work" by author
3. Branch gets updated by author
4. Referee (sits next to author, perhaps) agrees with changes and set ticket to "positive review" (he may have witnessed testing before step 2 already)
5. RM finishes testing/merging and if he/she checks at all, would find the ticket at "positive review" as expected and now moves status to "closed", but has only merged the old branch.

The obvious solution is to change step 1 to:
 1'. RM sets status of ticket to "merging" and starts testing/merging process (this change in status should be made by the same script that starts the testing merging process, in which case it would not add to the workload of the RM, only to a more elaborate ticket history)

I don't think the extra status would even need to be protected so that the RM can only change it to something else: the flag should be enough.


Volker Braun

unread,
Apr 15, 2015, 11:50:45 PM4/15/15
to sage-...@googlegroups.com
You are just renaming the step where nobody is supposed to make changes any more (from positive review to merging). It doesn't solve anything. Eventually a CI script will have to be written, and it'll automatically switch from positive review to merging. 

We could add a mandatory 2-week cooling off period while I'm not merging positively-reviewed tickets, if thats what you want. But I'd find that annoying.

Clemens Heuberger

unread,
Apr 16, 2015, 1:22:09 AM4/16/15
to sage-...@googlegroups.com
out of interest, I downloaded

http://trac.sagemath.org/query?status=closed&col=id&col=summary&col=status&col=milestone&col=resolution&col=changetime&col=commit&order=priority

as a .csv file and wrote an ugly script to see whether the commitid (if present) is merged in 6.7.beta0. I then manually removed those tickets which where closed yesterday, because that would be the tickets already closed for 6.7.beta1.

Here is the result

ticket commit changed
15017 872e2bc0225b6929ed8cc052a63850c892abe723 07/21/14 03:51:36
15660 d4842426356568815b80323f8fc6afbb80185ba3 01/25/15 14:55:21
15773 81a2ea983e2903b9cc5f2a78a57dd24fe32e48d5 08/31/14 19:45:31
16939 d9214c8c4ce4cbb5b29d207e8ed3bbf4e356b180 03/06/15 20:28:12
17221 2e62790eeec6463562120c204a86ebb323fa7cba 04/14/15 21:44:07
17307 7fa0de34aa713520d0fef2a7065581599dfd7bf3 11/09/14 20:40:11
14880 3b6a841dc5f6210a9c0d70b4d6cf24e34788c228 10/02/13 08:35:34
15599 08be4423f7703847d4da7c6d8cce4bb0902f93ed 03/13/14 04:38:46
16403 9f08c6b859a0fb853f69dbaedc73811740ee3e91 01/25/15 14:56:02
16847 e42ce551e66ff3f854c2424561bc71a075bac67a 08/19/14 12:38:59
16698 ba38202cf55caefb032e1b4519c96b9706577004 09/15/14 16:56:43
17702 daad37297eff79d7ee8c34989840d44f7e3e5105 02/08/15 17:31:25

#17221 (which started this thread) already has a successor ticket #18206; I did not have a closer look at the others.

And for completeness, here is the ugly script (it assumes that develop is the current branch):

import csv
import subprocess

with open('/tmp/query.csv', 'r') as csvfile:
tracreader = csv.reader(csvfile, delimiter=',', quotechar='"')
headings_row = next(tracreader)
headings_row[0] = 'id' # override UTF8 markers
for row in tracreader:
data = dict(zip(headings_row, row))
if data['commit'] and data['resolution'] == 'fixed':
try:
output = subprocess.check_output(["git", "branch", "--contains", data['commit']],
stderr=subprocess.STDOUT).split('\n')
except subprocess.CalledProcessError:
print data['id'], data['commit'], data['changetime']
if "* develop" not in output:
print data['id'], data['commit'], data['changetime']

I am aware of the fact that this would be cleaner if implemented in git trac; but for a first overview, this seemed to be the quicker approach.

Regards,

CH

Jeroen Demeyer

unread,
Apr 16, 2015, 1:49:03 AM4/16/15
to sage-...@googlegroups.com
On 2015-04-15 23:28, Daniel Krenn wrote:
> If there is a script for closing tickets, it could be extended to check
> if the status is still positive_review and if the current branch on the
> ticket was already (really) merged into develop...

Just FYI: that's essentially what I did. My script kept a file with
md5sums of the patches (remember those?) which were tested on the
buildbot. When the time came to actually close tickets, the md5sums of
the patches on the ticket were compared to the stored md5sums. A
mismatch would be an error.

Jeroen.

Jeroen Demeyer

unread,
Apr 16, 2015, 1:58:32 AM4/16/15
to sage-...@googlegroups.com
On 2015-04-16 01:33, Nils Bruin wrote:
> The whole meaning of "positive review" goes out of
> the window if one still changes the branch on the ticket.
I disagree. It can easily happen that a problem is found after a ticket
was set to positive_review. Perhaps some corner case doesn't quite work.
Or there could be trivial changes like a documentation typo.

Volker Braun

unread,
Apr 16, 2015, 2:03:48 AM4/16/15
to sage-...@googlegroups.com
Then open a new ticket. Which is exactly what you would do if the ticket has been switched to closed, so why is it impossible to do when it is positive_review?

Clemens Heuberger

unread,
Apr 16, 2015, 5:30:30 AM4/16/15
to sage-...@googlegroups.com

Out of my previous list, there were some false positives (I somehow mishandled
invalid tickets). Five incompletely merged tickets remained.

Am 2015-04-16 um 07:20 schrieb Clemens Heuberger:
> ticket commit changed
> 15017 872e2bc0225b6929ed8cc052a63850c892abe723 07/21/14 03:51:36

opened #18218 with the missing commit.

> 17307 7fa0de34aa713520d0fef2a7065581599dfd7bf3 11/09/14 20:40:11

Last commit was an amend changing the author of the commit, this is now unfixable.

> 14880 3b6a841dc5f6210a9c0d70b4d6cf24e34788c228 10/02/13 08:35:34

I do not understand that one, was merged in 5.13.beta0

> 15599 08be4423f7703847d4da7c6d8cce4bb0902f93ed 03/13/14 04:38:46

only a merge commit missing, therefore no new ticket.

> 16847 e42ce551e66ff3f854c2424561bc71a075bac67a 08/19/14 12:38:59

opened #18219 with the missing commit.

Both #18218 and #18219 are trivial. I used the authors of the missing authors
for trac's author field and set them to needs_review.

Regards, CH


kcrisman

unread,
Apr 16, 2015, 1:28:07 PM4/16/15
to sage-...@googlegroups.com
 

On Thursday, April 16, 2015 at 7:58:32 AM UTC+2, Jeroen Demeyer wrote:
On 2015-04-16 01:33, Nils Bruin wrote:
> The whole meaning of "positive review" goes out of
> the window if one still changes the branch on the ticket.
I disagree. It can easily happen that a problem is found after a ticket
was set to positive_review. Perhaps some corner case doesn't quite work.
Or there could be trivial changes like a documentation typo.



Then open a new ticket. Which is exactly what you would do if the ticket has been switched to closed, so why is it impossible to do when it is positive_review?


 Because of the social problem that people probably won't fix it, or at least not by the next stable release (surely not all such tickets would become blockers).  Then we'll have (for instance) some new behavior in Sage with an annoying bug that it totally reproducible but not taken care of, instead of just not having that in Sage at all, buggy or no.

That said, now that I see part of the problem is not just people changing tickets, but changing *branches* and *keeping* positive review (or something analogous to this), I agree that is a different situation.  So, here's my possibly buggy suggestion:

* If you actually HAVE a solution to the problem you see in a positive review ticket, then open a new ticket and put it there.
* If not, undo positive review and put to "needs work" and presumably it won't get fixed fast enough to matter.  Or if someone gets a fix, they could put it on a new ticket.  (?)

Clemens Heuberger

unread,
Apr 16, 2015, 2:29:52 PM4/16/15
to sage-...@googlegroups.com
Am 2015-04-16 um 19:28 schrieb kcrisman:
> That said, now that I see part of the problem is not just people changing
> tickets, but changing *branches* and *keeping* positive review (or something
> analogous to this), I agree that is a different situation. So, here's my
> possibly buggy suggestion:

technically, they do not *keep* positive review. They set it to needs_work,
needs_review, somebody reviews it, sets it to positive_review. So the ticket
went through a full cycle, but the earlier version is merged. And those tickets
where this actually happened were usually very minor issues (where the authors
presumably had the impression that opening an extra ticket is overkill).

The whole discussion was about the semantics of "positive_review". In the
release managers workflow, is actually closer to "closed" than I previously
assumed, but all other proposal essentially boiled down to introducing one more
status of a ticket. Thus it is all about the semantics of the available ticket
status.

We can then as well codify the current workflow.

Therefore, I now created #18228 to change the developer guide to state that one
should not push commits to a ticket set to positive review. This ticket needs
review.

Regards, CH
Reply all
Reply to author
Forward
0 new messages