[FIX] runbot_merge: better handle targets being branch-protected

If a staging covers multiple repositories and there's a fast-forward
issue on any but the first repo/target, runbot_merge attempted to
revert the commits it had fast-forwarded on the previous repos.

This doesn't work when branch-protection is active, unless runbot_merge
is a repository administrator (and branch protection is not configured
to apply to those): reverting is done by push-forcing the original head
back onto the ref, which branch-protection unconditionally precludes.

This commit does not entirely fix the race condition (it does not look
like github provides any way to do that), but it should significantly
reduce the race-condition window as it performs a semi-wet run of the
fast-forward process on the tmp branches before actually updating the
targets. That way the only remaining breakage should be when somebody
pushes on repositories 1.. between the test-FF on tmp branches and the
actual fast forward.

While at it, updated the github API thing to *always* dump the JSON body
on an error response, if the content-type is json.

runbot_merge/github.py

@@ -31,11 +31,7 @@ class GH(object):
                 exc = check.get(r.status_code)
                 if exc:
                     raise exc(r.content)
-            if r.status_code == 422:
+            if r.status_code >= 400 and r.headers.get('content-type', '').startswith('application/javascript'):
-                # dump & format body if it's a 422 as GH's HTTP Reason is
-                # completely useless (only states
-                # "Unprocessable Entity for URL: <endpoint>" which is not
-                # exactly great for debugging what went wrong
                 raise requests.HTTPError(
                     json_.dumps(r.json(), indent=4),
                     response=r

runbot_merge/models/pull_requests.py

@@ -74,14 +74,28 @@ class Project(models.Model):
                     staging, staging.state
                 )
                 if staging.state == 'success':
-                    old_heads = {
-                        n: g.head(staging.target.name)
-                        for n, g in gh.items()
-                    }
                     repo_name = None
                     staging_heads = json.loads(staging.heads)
-                    updated = []
                     try:
+                        # reverting updates doesn't work if the branches are
+                        # protected (because a revert is basically a force
+                        # push), instead use the tmp branch as a dry-run
+                        tmp_target = 'tmp.' + staging.target.name
+                        # first force-push the current targets to all tmps
+                        for repo_name in staging_heads.keys():
+                            if repo_name.endswith('^'):
+                                continue
+                            g = gh[repo_name]
+                            g.set_ref(tmp_target, g.head(staging.target.name))
+
+                        # then attempt to FF the tmp to the staging
+                        for repo_name, head in staging_heads.items():
+                            if repo_name.endswith('^'):
+                                continue
+                            gh[repo_name].fast_forward(tmp_target, staging_heads.get(repo_name + '^') or head)
+
+                        # there is still a race condition here, but it's way
+                        # lower than "the entire staging duration"...
                         for repo_name, head in staging_heads.items():
                             if repo_name.endswith('^'):
                                 continue
@@ -92,16 +106,12 @@ class Project(models.Model):
                                 staging.target.name,
                                 staging_heads.get(repo_name + '^') or head
                             )
-                            updated.append(repo_name)
                     except exceptions.FastForwardError as e:
                         logger.warning(
-                            "Could not fast-forward successful staging on %s:%s, reverting updated repos %s and re-staging",
+                            "Could not fast-forward successful staging on %s:%s",
                             repo_name, staging.target.name,
-                            ', '.join(updated),
                             exc_info=True
                         )
-                        for name in reversed(updated):
-                            gh[name].set_ref(staging.target.name, old_heads[name])
                         staging.write({
                             'state': 'ff_failed',
                             'reason': str(e.__cause__ or e.__context__ or '')

runbot_merge/tests/fake_github/__init__.py

@@ -94,6 +94,7 @@ class Repo(object):
         self.refs = {}
         # {event: (wsgi_app, url)}
         self.hooks = collections.defaultdict(list)
+        self.protected = set()
 
     def hook(self, hook, events):
         for event in events:
@@ -118,12 +119,34 @@ class Repo(object):
         assert 'heads/%s' % target in self.refs
         return PR(self, title, body, target, ctid, user=user, label='{}:{}'.format(user, label or target))
 
+    def get_ref(self, ref):
+        if re.match(r'[0-9a-f]{40}', ref):
+            return ref
+
+        sha = self.refs.get(ref)
+        assert sha, "no ref %s" % ref
+        return sha
+
     def make_ref(self, name, commit, force=False):
         assert isinstance(self.objects[commit], Commit)
         if not force and name in self.refs:
             raise ValueError("ref %s already exists" % name)
         self.refs[name] = commit
 
+    def protect(self, branch):
+        ref = 'heads/%s' % branch
+        assert ref in self.refs
+        self.protected.add(ref)
+
+    def update_ref(self, name, commit, force=False):
+        current = self.refs.get(name)
+        assert current is not None
+
+        assert name not in self.protected and force or git.is_ancestor(
+            self.objects, current, commit)
+
+        self.make_ref(name, commit, force=True)
+
     def commit(self, ref):
         sha = self.refs.get(ref) or ref
         commit = self.objects[sha]
@@ -252,11 +275,11 @@ class Repo(object):
         if sha not in self.objects:
             return (404, None)
 
-        if not body.get('force'):
+        try:
-            if not git.is_ancestor(self.objects, current, sha):
+            self.update_ref(ref, sha, body.get('force') or False)
-                return (400, None)
+        except AssertionError:
+            return (400, None)
 
-        self.make_ref(ref, sha, force=True)
         return (200, {
             "ref": "refs/%s" % ref,
             "object": {

runbot_merge/tests/remote.py

@@ -446,6 +446,16 @@ class Repo:
         assert 200 <= r.status_code < 300, r.json()
         wait_for_hook()
 
+    def protect(self, branch):
+        r = self._session.put('https://api.github.com/repos/{}/branches/{}/protection'.format(self.name, branch), json={
+            'required_status_checks': None,
+            'enforce_admins': True,
+            'required_pull_request_reviews': None,
+            'restrictions': None,
+        })
+        assert 200 <= r.status_code < 300, r.json()
+        wait_for_hook()
+
     def update_ref(self, name, commit, force=False):
         r = self._session.patch('https://api.github.com/repos/{}/git/refs/{}'.format(self.name, name), json={'sha': commit, 'force': force})
         assert 200 <= r.status_code < 300, r.json()

runbot_merge/tests/test_basic.py

@@ -5,6 +5,7 @@ import time
 import requests
 
 import pytest
+from requests import HTTPError
 
 import odoo
 
@@ -1913,6 +1914,20 @@ class TestComments:
         assert {p.github_login for p in pr.delegates} \
             == {'foo', 'bar', 'baz'}
 
+class TestInfrastructure:
+    def test_protection(self, repo):
+        """ force-pushing on a protected ref should fail
+        """
+        m0 = repo.make_commit(None, 'initial', None, tree={'m': 'm0'})
+        m1 = repo.make_commit(m0, 'first', None, tree={'m': 'm1'})
+        repo.make_ref('heads/master', m1)
+        repo.protect('master')
+
+        c1 = repo.make_commit(m0, 'other', None, tree={'m': 'c1'})
+        with pytest.raises(AssertionError):
+            repo.update_ref('heads/master', c1, force=True)
+        assert repo.get_ref('heads/master') == m1
+
 def node(name, *children):
     assert type(name) in (str, re_matches)
     return name, frozenset(children)

runbot_merge/tests/test_multirepo.py

@@ -64,12 +64,14 @@ def test_stage_one(env, project, repo_a, repo_b):
         'heads/master',
         repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'})
     )
+    repo_a.protect('master')
     pr_a = make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing')
 
     repo_b.make_ref(
         'heads/master',
         repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'})
     )
+    repo_b.protect('master')
     pr_b = make_pr(repo_b, 'B', [{'a': 'b_1'}], label='do-other-thing')
 
     env['runbot_merge.project']._check_progress()
@@ -87,12 +89,14 @@ def test_stage_match(env, project, repo_a, repo_b):
         'heads/master',
         repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'})
     )
+    repo_a.protect('master')
     pr_a = make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing')
 
     repo_b.make_ref(
         'heads/master',
         repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'})
     )
+    repo_b.protect('master')
     pr_b = make_pr(repo_b, 'B', [{'a': 'b_1'}], label='do-a-thing')
 
     env['runbot_merge.project']._check_progress()
@@ -115,18 +119,21 @@ def test_sub_match(env, project, repo_a, repo_b, repo_c):
         'heads/master',
         repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'})
     )
+    repo_a.protect('master')
     # no pr here
 
     repo_b.make_ref(
         'heads/master',
         repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'})
     )
+    repo_b.protect('master')
     pr_b = make_pr(repo_b, 'B', [{'a': 'b_1'}], label='do-a-thing')
 
     repo_c.make_ref(
         'heads/master',
         repo_c.make_commit(None, 'initial', None, tree={'a': 'c_0'})
     )
+    repo_c.protect('master')
     pr_c = make_pr(repo_c, 'C', [{'a': 'c_1'}], label='do-a-thing')
 
     env['runbot_merge.project']._check_progress()
@@ -161,8 +168,10 @@ def test_merge_fail(env, project, repo_a, repo_b, users):
 
     root_a = repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'})
     repo_a.make_ref('heads/master', root_a)
+    repo_a.protect('master')
     root_b = repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'})
     repo_b.make_ref('heads/master', root_b)
+    repo_b.protect('master')
 
     # first set of matched PRs
     pr1a = make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing')
@@ -206,10 +215,12 @@ def test_ff_fail(env, project, repo_a, repo_b):
     project.batch_limit = 1
     root_a = repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'})
     repo_a.make_ref('heads/master', root_a)
+    repo_a.protect('master')
     make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing')
 
     root_b = repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'})
     repo_b.make_ref('heads/master', root_b)
+    repo_b.protect('master')
     make_pr(repo_b, 'B', [{'a': 'b_1'}], label='do-a-thing')
 
     env['runbot_merge.project']._check_progress()
@@ -241,11 +252,13 @@ def test_one_failed(env, project, repo_a, repo_b, owner):
     project.batch_limit = 1
     c_a = repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'})
     repo_a.make_ref('heads/master', c_a)
+    repo_a.protect('master')
     # pr_a is born ready
     pr_a = make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing')
 
     c_b = repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'})
     repo_b.make_ref('heads/master', c_b)
+    repo_b.protect('master')
     c_pr = repo_b.make_commit(c_b, 'pr', None, tree={'a': 'b_1'})
     pr_b = repo_b.make_pr(
         'title', 'body', target='master', ctid=c_pr,
@@ -273,6 +286,7 @@ def test_other_failed(env, project, repo_a, repo_b, owner, users):
     """
     c_a = repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'})
     repo_a.make_ref('heads/master', c_a)
+    repo_a.protect('master')
     # pr_a is born ready
     pr_a = make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing')
     repo_a.post_status(pr_a.head, 'success', 'ci/runbot')
@@ -280,6 +294,7 @@ def test_other_failed(env, project, repo_a, repo_b, owner, users):
 
     c_b = repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'})
     repo_b.make_ref('heads/master', c_b)
+    repo_b.protect('master')
 
     env['runbot_merge.project']._check_progress()
     pr = to_pr(env, pr_a)
@@ -305,7 +320,9 @@ def test_batching(env, project, repo_a, repo_b):
     """
     project.batch_limit = 3
     repo_a.make_ref('heads/master', repo_a.make_commit(None, 'initial', None, tree={'a': 'a0'}))
+    repo_a.protect('master')
     repo_b.make_ref('heads/master', repo_b.make_commit(None, 'initial', None, tree={'b': 'b0'}))
+    repo_b.protect('master')
 
     prs = [(
         a and to_pr(env, make_pr(repo_a, 'A{}'.format(i), [{'a{}'.format(i): 'a{}'.format(i)}], label='batch{}'.format(i))),
@@ -334,7 +351,9 @@ def test_batching_split(env, repo_a, repo_b):
     """ If a staging fails, it should get split properly across repos
     """
     repo_a.make_ref('heads/master', repo_a.make_commit(None, 'initial', None, tree={'a': 'a0'}))
+    repo_a.protect('master')
     repo_b.make_ref('heads/master', repo_b.make_commit(None, 'initial', None, tree={'b': 'b0'}))
+    repo_b.protect('master')
 
     prs = [(
         a and to_pr(env, make_pr(repo_a, 'A{}'.format(i), [{'a{}'.format(i): 'a{}'.format(i)}], label='batch{}'.format(i))),
@@ -377,7 +396,9 @@ def test_urgent(env, repo_a, repo_b):
     being prioritized
     """
     repo_a.make_ref('heads/master', repo_a.make_commit(None, 'initial', None, tree={'a0': 'a'}))
+    repo_a.protect('master')
     repo_b.make_ref('heads/master', repo_b.make_commit(None, 'initial', None, tree={'b0': 'b'}))
+    repo_b.protect('master')
 
     pr_a = make_pr(repo_a, 'A', [{'a1': 'a'}, {'a2': 'a'}], label='batch', reviewer=None, statuses=[])
     pr_b = make_pr(repo_b, 'B', [{'b1': 'b'}, {'b2': 'b'}], label='batch', reviewer=None, statuses=[])