From 2e297b09740df9ef031c91e8c2e7dbede91f63b5 Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
Date: Jan 16 2017 16:18:36 +0000
Subject: [PATCH 1/5] Make updating git repos a locked process


This way we should prevent two processes/requests updating the git repo
at the same time and potentially running into each other which may lead
to a broken history in the repo where object are pushed incorrectly.

---

diff --git a/pagure/lib/git.py b/pagure/lib/git.py
index a27e083..f460416 100644
--- a/pagure/lib/git.py
+++ b/pagure/lib/git.py
@@ -24,6 +24,7 @@ import subprocess
 import tempfile
 
 import arrow
+import filelock
 import pygit2
 import werkzeug
 
@@ -169,80 +170,87 @@ def update_git(obj, repo, repofolder):
 
     # Get the fork
     repopath = os.path.join(repofolder, repo.path)
+    lockfile = '%s.lock' % repopath
+
+    lock = filelock.FileLock(lockfile)
+    with lock:
+
+        # Clone the repo into a temp folder
+        newpath = tempfile.mkdtemp(prefix='pagure-')
+        new_repo = pygit2.clone_repository(repopath, newpath)
+
+        file_path = os.path.join(newpath, obj.uid)
+
+        # Get the current index
+        index = new_repo.index
+
+        # Are we adding files
+        added = False
+        if not os.path.exists(file_path):
+            added = True
+
+        # Write down what changed
+        with open(file_path, 'w') as stream:
+            stream.write(json.dumps(
+                obj.to_json(), sort_keys=True, indent=4,
+                separators=(',', ': ')))
+
+        # Retrieve the list of files that changed
+        diff = new_repo.diff()
+        files = []
+        for patch in diff:
+            if hasattr(patch, 'new_file_path'):
+                files.append(patch.new_file_path)
+            elif hasattr(patch, 'delta'):
+                files.append(patch.delta.new_file.path)
+
+        # Add the changes to the index
+        if added:
+            index.add(obj.uid)
+        for filename in files:
+            index.add(filename)
+
+        # If not change, return
+        if not files and not added:
+            shutil.rmtree(newpath)
+            os.unlink(lockfile)
+            return
 
-    # Clone the repo into a temp folder
-    newpath = tempfile.mkdtemp(prefix='pagure-')
-    new_repo = pygit2.clone_repository(repopath, newpath)
-
-    file_path = os.path.join(newpath, obj.uid)
-
-    # Get the current index
-    index = new_repo.index
-
-    # Are we adding files
-    added = False
-    if not os.path.exists(file_path):
-        added = True
-
-    # Write down what changed
-    with open(file_path, 'w') as stream:
-        stream.write(json.dumps(
-            obj.to_json(), sort_keys=True, indent=4,
-            separators=(',', ': ')))
-
-    # Retrieve the list of files that changed
-    diff = new_repo.diff()
-    files = []
-    for patch in diff:
-        if hasattr(patch, 'new_file_path'):
-            files.append(patch.new_file_path)
-        elif hasattr(patch, 'delta'):
-            files.append(patch.delta.new_file.path)
-
-    # Add the changes to the index
-    if added:
-        index.add(obj.uid)
-    for filename in files:
-        index.add(filename)
-
-    # If not change, return
-    if not files and not added:
-        shutil.rmtree(newpath)
-        return
-
-    # See if there is a parent to this commit
-    parent = None
-    try:
-        parent = new_repo.head.get_object().oid
-    except pygit2.GitError:
-        pass
-
-    parents = []
-    if parent:
-        parents.append(parent)
-
-    # Author/commiter will always be this one
-    author = pygit2.Signature(name='pagure', email='pagure')
-
-    # Actually commit
-    new_repo.create_commit(
-        'refs/heads/master',
-        author,
-        author,
-        'Updated %s %s: %s' % (obj.isa, obj.uid, obj.title),
-        new_repo.index.write_tree(),
-        parents)
-    index.write()
-
-    # Push to origin
-    ori_remote = new_repo.remotes[0]
-    master_ref = new_repo.lookup_reference('HEAD').resolve()
-    refname = '%s:%s' % (master_ref.name, master_ref.name)
+        # See if there is a parent to this commit
+        parent = None
+        try:
+            parent = new_repo.head.get_object().oid
+        except pygit2.GitError:
+            pass
+
+        parents = []
+        if parent:
+            parents.append(parent)
+
+        # Author/commiter will always be this one
+        author = pygit2.Signature(name='pagure', email='pagure')
+
+        # Actually commit
+        new_repo.create_commit(
+            'refs/heads/master',
+            author,
+            author,
+            'Updated %s %s: %s' % (obj.isa, obj.uid, obj.title),
+            new_repo.index.write_tree(),
+            parents)
+        index.write()
+
+        # Push to origin
+        ori_remote = new_repo.remotes[0]
+        master_ref = new_repo.lookup_reference('HEAD').resolve()
+        refname = '%s:%s' % (master_ref.name, master_ref.name)
 
-    PagureRepo.push(ori_remote, refname)
+        PagureRepo.push(ori_remote, refname)
 
     # Remove the clone
     shutil.rmtree(newpath)
+    # Remove the lock file
+    os.unlink(lockfile)
 
 
 def clean_git(obj, repo, repofolder):
@@ -835,84 +843,93 @@ def update_file_in_git(
     # Get the fork
     repopath = pagure.get_repo_path(repo)
 
-    # Clone the repo into a temp folder
-    newpath = tempfile.mkdtemp(prefix='pagure-')
-    new_repo = pygit2.clone_repository(
-        repopath, newpath, checkout_branch=branch)
+    lockfile = '%s.lock' % repopath
 
-    file_path = os.path.join(newpath, filename)
+    lock = filelock.FileLock(lockfile)
+    with lock:
 
-    # Get the current index
-    index = new_repo.index
+        # Clone the repo into a temp folder
+        newpath = tempfile.mkdtemp(prefix='pagure-')
+        new_repo = pygit2.clone_repository(
+            repopath, newpath, checkout_branch=branch)
 
-    # Write down what changed
-    with open(file_path, 'w') as stream:
-        stream.write(content.replace('\r', '').encode('utf-8'))
-
-    # Retrieve the list of files that changed
-    diff = new_repo.diff()
-    files = []
-    for patch in diff:
-        if hasattr(patch, 'new_file_path'):
-            files.append(patch.new_file_path)
-        elif hasattr(patch, 'delta'):
-            files.append(patch.delta.new_file.path)
-
-    # Add the changes to the index
-    added = False
-    for filename in files:
-        added = True
-        index.add(filename)
-
-    # If not change, return
-    if not files and not added:
-        shutil.rmtree(newpath)
-        return
+        file_path = os.path.join(newpath, filename)
 
-    # See if there is a parent to this commit
-    branch_ref = get_branch_ref(new_repo, branch)
-    parent = branch_ref.get_object()
+        # Get the current index
+        index = new_repo.index
 
-    # See if we need to create the branch
-    nbranch_ref = None
-    if branchto not in new_repo.listall_branches():
-        nbranch_ref = new_repo.create_branch(branchto, parent)
+        # Write down what changed
+        with open(file_path, 'w') as stream:
+            stream.write(content.replace('\r', '').encode('utf-8'))
 
-    parents = []
-    if parent:
-        parents.append(parent.hex)
+        # Retrieve the list of files that changed
+        diff = new_repo.diff()
+        files = []
+        for patch in diff:
+            if hasattr(patch, 'new_file_path'):
+                files.append(patch.new_file_path)
+            elif hasattr(patch, 'delta'):
+                files.append(patch.delta.new_file.path)
 
-    # Author/commiter will always be this one
-    author = pygit2.Signature(
-        name=user.username.encode('utf-8'),
-        email=email.encode('utf-8')
-    )
+        # Add the changes to the index
+        added = False
+        for filename in files:
+            added = True
+            index.add(filename)
 
-    # Actually commit
-    new_repo.create_commit(
-        nbranch_ref.name if nbranch_ref else branch_ref.name,
-        author,
-        author,
-        message.strip(),
-        new_repo.index.write_tree(),
-        parents)
-    index.write()
+        # If not change, return
+        if not files and not added:
+            shutil.rmtree(newpath)
+            os.unlink(lockfile)
+            return
+
+        # See if there is a parent to this commit
+        branch_ref = get_branch_ref(new_repo, branch)
+        parent = branch_ref.get_object()
+
+        # See if we need to create the branch
+        nbranch_ref = None
+        if branchto not in new_repo.listall_branches():
+            nbranch_ref = new_repo.create_branch(branchto, parent)
+
+        parents = []
+        if parent:
+            parents.append(parent.hex)
+
+        # Author/commiter will always be this one
+        author = pygit2.Signature(
+            name=user.username.encode('utf-8'),
+            email=email.encode('utf-8')
+        )
 
-    # Push to origin
-    ori_remote = new_repo.remotes[0]
-    refname = '%s:refs/heads/%s' % (
-        nbranch_ref.name if nbranch_ref else branch_ref.name,
-        branchto)
+        # Actually commit
+        new_repo.create_commit(
+            nbranch_ref.name if nbranch_ref else branch_ref.name,
+            author,
+            author,
+            message.strip(),
+            new_repo.index.write_tree(),
+            parents)
+        index.write()
+
+        # Push to origin
+        ori_remote = new_repo.remotes[0]
+        refname = '%s:refs/heads/%s' % (
+            nbranch_ref.name if nbranch_ref else branch_ref.name,
+            branchto)
 
-    try:
-        PagureRepo.push(ori_remote, refname)
-    except pygit2.GitError as err:  # pragma: no cover
-        shutil.rmtree(newpath)
-        raise pagure.exceptions.PagureException(
-            'Commit could not be done: %s' % err)
+        try:
+            PagureRepo.push(ori_remote, refname)
+        except pygit2.GitError as err:  # pragma: no cover
+            os.unlink(lockfile)
+            shutil.rmtree(newpath)
+            raise pagure.exceptions.PagureException(
+                'Commit could not be done: %s' % err)
 
     # Remove the clone
     shutil.rmtree(newpath)
+    # Remove the lock file
+    os.unlink(lockfile)
 
     return os.path.join('files', filename)
 

From 66afc99f18a2cbd6d65a9bc123b08eb0c079bf50 Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
Date: Jan 16 2017 16:18:36 +0000
Subject: [PATCH 2/5] Add the dependency to filelock


---

diff --git a/files/pagure.spec b/files/pagure.spec
index 0c25839..80d6d17 100644
--- a/files/pagure.spec
+++ b/files/pagure.spec
@@ -25,6 +25,7 @@ BuildRequires:      python-blinker
 BuildRequires:      python-chardet
 BuildRequires:      python-cryptography
 BuildRequires:      python-docutils
+BuildRequires:      python-filelock
 BuildRequires:      python-flask
 BuildRequires:      python-flask-wtf
 BuildRequires:      python-flask-multistatic
@@ -62,6 +63,7 @@ Requires:           python-chardet
 Requires:           python-cryptography
 Requires:           python-docutils
 Requires:           python-enum34
+Requires:           python-filelock
 Requires:           python-flask
 Requires:           python-flask-wtf
 Requires:           python-flask-multistatic
diff --git a/requirements.txt b/requirements.txt
index 907b94c..515f0fd 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -8,6 +8,7 @@ blinker
 chardet
 docutils
 enum34
+filelock
 flask
 flask-wtf
 flask-multistatic

From 36b67be696dd301fac52834e9b8897908ae2e70b Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
Date: Jan 16 2017 16:18:36 +0000
Subject: [PATCH 3/5] Catch the timeout exception filelock will raise if it takes too long


---

diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py
index c1837b8..4a2fc85 100644
--- a/pagure/ui/fork.py
+++ b/pagure/ui/fork.py
@@ -20,6 +20,7 @@ import flask
 import os
 from math import ceil
 
+import filelock
 import pygit2
 from sqlalchemy.exc import SQLAlchemyError
 
@@ -535,9 +536,20 @@ def pull_request_add_comment(
         except SQLAlchemyError as err:  # pragma: no cover
             SESSION.rollback()
             APP.logger.exception(err)
-            flask.flash(str(err), 'error')
             if is_js:
                 return 'error'
+            else:
+                flask.flash(str(err), 'error')
+        except filelock.Timeout as err:  # pragma: no cover
+            is_js = False
+            SESSION.rollback()
+            APP.logger.exception(err)
+            if is_js:
+                return 'error'
+            else:
+                flask.flash(
+                    'We could not save all the info, please try again',
+                    'error')
 
         if is_js:
             return 'ok'
diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py
index 136cfa4..eb5196a 100644
--- a/pagure/ui/issues.py
+++ b/pagure/ui/issues.py
@@ -21,6 +21,7 @@ import re
 from collections import defaultdict
 from math import ceil
 
+import filelock
 import pygit2
 import werkzeug.datastructures
 from sqlalchemy.exc import SQLAlchemyError
@@ -327,6 +328,14 @@ def update_issue(repo, issueid, username=None, namespace=None):
             APP.logger.exception(err)
             if not is_js:
                 flask.flash(str(err), 'error')
+        except filelock.Timeout as err:  # pragma: no cover
+            is_js = False
+            SESSION.rollback()
+            APP.logger.exception(err)
+            if not is_js:
+                flask.flash(
+                    'We could not save all the info, please try again',
+                    'error')
     else:
         if is_js:
             return 'notok: %s' % form.errors

From 08eaf87f8482de75aa698acbc80d4733d197e0d1 Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
Date: Jan 16 2017 16:18:36 +0000
Subject: [PATCH 4/5] Fix indentation for removing the fork inside the block where it is defined


---

diff --git a/pagure/lib/git.py b/pagure/lib/git.py
index f460416..d55be94 100644
--- a/pagure/lib/git.py
+++ b/pagure/lib/git.py
@@ -247,8 +247,9 @@ def update_git(obj, repo, repofolder):
 
         PagureRepo.push(ori_remote, refname)
 
-    # Remove the clone
-    shutil.rmtree(newpath)
+        # Remove the clone
+        shutil.rmtree(newpath)
+
     # Remove the lock file
     os.unlink(lockfile)
 
@@ -926,8 +927,9 @@ def update_file_in_git(
             raise pagure.exceptions.PagureException(
                 'Commit could not be done: %s' % err)
 
-    # Remove the clone
-    shutil.rmtree(newpath)
+        # Remove the clone
+        shutil.rmtree(newpath)
+
     # Remove the lock file
     os.unlink(lockfile)
 

From 1cac3412abb4837210acc1f0d38e48bf21de3d5a Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
Date: Jan 16 2017 16:18:36 +0000
Subject: [PATCH 5/5] Catch in more places the potential filelock.Timeout exception


This exception is raised when the filelock reaches a given timeout (20
seconds by default), so in every place we are interacting with a git repo
we need to catch this exception.

---

diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py
index 4a2fc85..d179eb3 100644
--- a/pagure/ui/fork.py
+++ b/pagure/ui/fork.py
@@ -541,7 +541,6 @@ def pull_request_add_comment(
             else:
                 flask.flash(str(err), 'error')
         except filelock.Timeout as err:  # pragma: no cover
-            is_js = False
             SESSION.rollback()
             APP.logger.exception(err)
             if is_js:
@@ -705,8 +704,18 @@ def pull_request_edit_comment(
             LOG.error(err)
             if is_js:
                 return 'error'
-            flask.flash(
-                'Could not edit the comment: %s' % commentid, 'error')
+            else:
+                flask.flash(
+                    'Could not edit the comment: %s' % commentid, 'error')
+        except filelock.Timeout as err:  # pragma: no cover
+            SESSION.rollback()
+            APP.logger.exception(err)
+            if is_js:
+                return 'error'
+            else:
+                flask.flash(
+                    'We could not save all the info, please try again',
+                    'error')
 
         if is_js:
             return 'ok'
@@ -854,6 +863,13 @@ def cancel_request_pull(repo, requestid, username=None, namespace=None):
             flask.flash(
                 'Could not update this pull-request in the database',
                 'error')
+        except filelock.Timeout as err:  # pragma: no cover
+            SESSION.rollback()
+            APP.logger.exception(err)
+            flask.flash(
+                    'We could not save all the info, please try again',
+                    'error')
+
     else:
         flask.flash('Invalid input submitted', 'error')
 
@@ -911,6 +927,12 @@ def set_assignee_requests(repo, requestid, username=None, namespace=None):
             SESSION.rollback()
             APP.logger.exception(err)
             flask.flash(str(err), 'error')
+        except filelock.Timeout as err:  # pragma: no cover
+            SESSION.rollback()
+            APP.logger.exception(err)
+            flask.flash(
+                'We could not save all the info, please try again',
+                'error')
 
     return flask.redirect(flask.url_for(
         'request_pull', username=username, namespace=namespace,
@@ -1079,6 +1101,12 @@ def new_request_pull(
         except SQLAlchemyError as err:  # pragma: no cover
             SESSION.rollback()
             flask.flash(str(err), 'error')
+        except filelock.Timeout as err:  # pragma: no cover
+            SESSION.rollback()
+            APP.logger.exception(err)
+            flask.flash(
+                    'We could not save all the info, please try again',
+                    'error')
 
     if not repo_admin:
         form = None
diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py
index eb5196a..631f579 100644
--- a/pagure/ui/issues.py
+++ b/pagure/ui/issues.py
@@ -320,20 +320,17 @@ def update_issue(repo, issueid, username=None, namespace=None):
         except pagure.exceptions.PagureException as err:
             is_js = False
             SESSION.rollback()
-            if not is_js:
-                flask.flash(err.message, 'error')
+            flask.flash(err.message, 'error')
         except SQLAlchemyError as err:  # pragma: no cover
             is_js = False
             SESSION.rollback()
             APP.logger.exception(err)
-            if not is_js:
-                flask.flash(str(err), 'error')
+            flask.flash(str(err), 'error')
         except filelock.Timeout as err:  # pragma: no cover
             is_js = False
             SESSION.rollback()
             APP.logger.exception(err)
-            if not is_js:
-                flask.flash(
+            flask.flash(
                     'We could not save all the info, please try again',
                     'error')
     else:
@@ -848,6 +845,12 @@ def new_issue(repo, username=None, namespace=None):
         except SQLAlchemyError as err:  # pragma: no cover
             SESSION.rollback()
             flask.flash(str(err), 'error')
+        except filelock.Timeout as err:  # pragma: no cover
+            SESSION.rollback()
+            APP.logger.exception(err)
+            flask.flash(
+                    'We could not save all the info, please try again',
+                    'error')
 
     types = None
     default = None
@@ -1093,10 +1096,18 @@ def edit_issue(repo, issueid, username=None, namespace=None):
                 repo=repo.name, issueid=issueid)
             return flask.redirect(url)
         except pagure.exceptions.PagureException as err:
+            SESSION.rollback()
             flask.flash(str(err), 'error')
         except SQLAlchemyError as err:  # pragma: no cover
             SESSION.rollback()
             flask.flash(str(err), 'error')
+        except filelock.Timeout as err:  # pragma: no cover
+            SESSION.rollback()
+            APP.logger.exception(err)
+            flask.flash(
+                    'We could not save all the info, please try again',
+                    'error')
+
     elif flask.request.method == 'GET':
         form.title.data = issue.title
         form.issue_content.data = issue.content
@@ -1147,14 +1158,22 @@ def upload_issue(repo, issueid, username=None, namespace=None):
 
     if form.validate_on_submit():
         filestream = flask.request.files['filestream']
-        new_filename = pagure.lib.git.add_file_to_git(
-            repo=repo,
-            issue=issue,
-            ticketfolder=APP.config['TICKETS_FOLDER'],
-            user=user_obj,
-            filename=filestream.filename,
-            filestream=filestream.stream,
-        )
+        try:
+            new_filename = pagure.lib.git.add_file_to_git(
+                repo=repo,
+                issue=issue,
+                ticketfolder=APP.config['TICKETS_FOLDER'],
+                user=user_obj,
+                filename=filestream.filename,
+                filestream=filestream.stream,
+            )
+        except filelock.Timeout as err:  # pragma: no cover
+            SESSION.rollback()
+            APP.logger.exception(err)
+            flask.flash(
+                    'We could not save all the info, please try again',
+                    'error')
+
         return flask.jsonify({
             'output': 'ok',
             'filename': new_filename.split('-', 1)[1],
@@ -1310,6 +1329,12 @@ def edit_comment_issue(
                 return 'error'
             flask.flash(
                 'Could not edit the comment: %s' % commentid, 'error')
+        except filelock.Timeout as err:  # pragma: no cover
+            SESSION.rollback()
+            APP.logger.exception(err)
+            flask.flash(
+                    'We could not save all the info, please try again',
+                    'error')
 
         if is_js:
             return 'ok'