possible race condition in add to collection feature

[mtholder]

I'm not sure if this is an opentree (curator) issue or phylesystem-api issue.
I think that if 2 users load studies and then each try to add a tree to a collection, I think that it is possible that one user's edits overwrite the others. See OpenTreeOfLife/collections-1@c10f75a as a possible example commit.
I tried to reproduce this by adding two trees. Only one got in ( OpenTreeOfLife/collections-1@b444470 ) and I did not see a commit for the other (or an error message indicating that the commit failed).
I would investigate further, but I have to go off line for a bit...

[jimallman]

Just to clarify, I'm seeing two commits from Apr 29 in the history:

• OpenTreeOfLife/collections-1@4feb1ff
• OpenTreeOfLife/collections-1@b444470

I gather you were expecting to see a third?

EDIT: Also, were you in fact acting as two different users, or as mtholder in all cases?

[jimallman]

I took a look for unmerged branches, and there are a few in collections-1 repo on api.opentreeoflife.org (looks like barnacles could use some cleanup):

~/repo/collections-1_par/collections-1$ git branch
  ewersSaucedo_collection_kcranston/barnacles_0
  kacrandall_collection_kcranston/barnacles_0
* master
  mtholder_collection_opentreeoflife/default_0

A closer look at the unmerged branch for opentreeoflife/default shows this:

$ git diff mtholder_collection_opentreeoflife/default_0 master
diff --git a/collections-by-owner/opentreeoflife/default.json b/collections-by-owner/opentreeoflife/default.json
index c7d6f11..b0afcc5 100644
--- a/collections-by-owner/opentreeoflife/default.json
+++ b/collections-by-owner/opentreeoflife/default.json
@@ -46,8 +46,8 @@
 "SHA": "",
 "comments": "",
 "decision": "INCLUDED",
-"name": "BEAST MCC (J\u00f8nsson, 2016)",
-"studyID": "ot_520",
+"name": "tree 3 (Shayla Salzman, 2015)",
+"studyID": "ot_711",
 "treeID": "tree1"
 }
 ],

Is it possible that you overlooked a warning about this in the more recent attempt to save changes?

[mtholder]

Yeah. sorry to be unclear. Looking at this in a little more depth...
I don't think that this is a race condition. I'll try in a few minutes to reproduce.

Some background: At the meeting in DC, I helped David Maddison added one of his trees and Chelsea Specht add one of hers. I thought both attempts were successful, but only Chelsea's tree was in the collection. Some how that I helped David with just added him to the list of curators for the collection (OpenTreeOfLife/collections-1@2cd4083) and did not add a tree. Neither he nor I noticed an error at the time.

Last night, I suspected that maybe Chelsea had loaded the collection list before David's commit made it in. So I added David's tree to correct for whatever the error was on the previous day.

Then I tried to replicate by loading two studies and adding a tree from each to the default collection without reloading. It looks like only one made it in. I was in a bit of a rush. I'll try again now and be more careful about looking for errors. First step is finding 2 trees that look like the both are ready to go into synth...

[jimallman]

I see that we're prone to these problems due to the way that we append new trees to the existing list. From git's point of view, this is a recipe for merge conflicts. I'll look around for ways to signal to git that these are distinct (but similar) changes.

[mtholder]

our last comments passed each other in the interwebs...

Yes. Very possible that I missed that note about not saving.

[mtholder]

I think the suggestion that I just made (in gitter) might help here.
If the curator saw a button "add this tree to synth" then on the backend it could be an atomic "add this tree to the end of the list".

[jimallman]

Yeah, I see how that could be an important safety measure in high-traffic collections. But ugh, such a special case.

[mtholder]

I'll make a separate issue, link to this, and then close it.

[jimallman]

and then close it

Close which? Meanwhile, I'm looking into git's clean/smudge filters, and whether we can configure these on GitHub. (We might be able to avoid these bugs by doing it just on the api server.) I'm also taking a quick look at word-diff and other alternatives to the default line-wise behavior of git-diff.

[mtholder]

sorry I meant close this one, because it is not a race. So this whole issue was a mess. Sorry for the static.

I think that the other issue (OpenTreeOfLife/phylesystem-api#175) is clearer on what we need.

I think that David and I some how failed to add his tree - so that example wasn't a work-in-progress issue. But when I tried to recreate the issue I failed to note the failure for my second change to make it onto the master branch.

[jimallman]

Just a quick comment on the original symptom reported above (missing commit). This could be a consequence of git's default "fast forward" behavior. This omits a merge commit if the result is the same as just moving the HEAD reference.

Or perhaps it could be the result of squashed commits..?