[FIX] mergebot: improve handling of having missed PR updates

1. if we try to stage a PR and realize we'd stored / checked the wrong
   head, cancel the staging and notify the PR
2. provide a command to forcefully update pr heads (or at least check
   that a PR's head is up to date)

Closes #241

runbot_merge/exceptions.py

@@ -2,3 +2,5 @@ class MergeError(Exception):
     pass
 class FastForwardError(Exception):
     pass
+class Skip(MergeError):
+    pass

runbot_merge/models/pull_requests.py

@@ -239,6 +239,24 @@ class Repository(models.Model):
             })
             return
 
+        # if the PR is already loaded, check... if the heads match?
+        pr_id = self.env['runbot_merge.pull_requests'].search([
+            ('repository.name', '=', pr['base']['repo']['full_name']),
+            ('number', '=', pr['number']),
+        ])
+        if pr_id:
+            # TODO: edited, maybe (requires crafting a 'changes' object)
+            r = controllers.handle_pr(self.env, {
+                'action': 'synchronize',
+                'pull_request': pr,
+            })
+            self.env['runbot_merge.pull_requests.feedback'].create({
+                'repository': pr_id.repository.id,
+                'pull_request': self.number,
+                'message': r,
+            })
+            return
+
         controllers.handle_pr(self.env, {
             'action': 'opened',
             'pull_request': pr,
@@ -641,8 +659,8 @@ class PullRequests(models.Model):
             commandstring,
         ):
             name, flag, param = m.groups()
-            if name == 'retry':
+            if name in ('retry', 'check'):
-                yield ('retry', None)
+                yield (name, None)
             elif name in ('r', 'review'):
                 if flag == '+':
                     yield ('review', True)
@@ -736,6 +754,12 @@ class PullRequests(models.Model):
                         self.state = 'ready'
                     else:
                         msg = "Retry makes no sense when the PR is not in error."
+            elif command == 'check':
+                if is_author:
+                    self.env['runbot_merge.fetch_job'].create({
+                        'repository': self.repository.id,
+                        'number': self.number,
+                    })
             elif command == 'review':
                 if param and is_reviewer:
                     newstate = RPLUS.get(self.state)
@@ -1074,6 +1098,9 @@ class PullRequests(models.Model):
             "rebasing a PR of more than 50 commits is a tad excessive"
         assert commits < 250, "merging PRs of 250+ commits is not supported (https://developer.github.com/v3/pulls/#list-commits-on-a-pull-request)"
         pr_commits = gh.commits(self.number)
+        pr_head = pr_commits[-1]['sha']
+        if pr_head != self.head:
+            raise exceptions.Skip(self.head, pr_head, commits == 1)
 
         if self.reviewed_by and self.reviewed_by.name == self.reviewed_by.github_login:
             # XXX: find other trigger(s) to sync github name?
@@ -1716,12 +1743,31 @@ class Batch(models.Model):
                     original_head, new_heads[pr]
                 )
             except (exceptions.MergeError, AssertionError) as e:
-                _logger.exception("Failed to merge %s:%s into staging branch (error: %s)", pr.repository.name, pr.number, e)
+                if isinstance(e, exceptions.Skip):
-                pr.state = 'error'
+                    old_head, new_head, to_squash = e.args
+                    pr.write({
+                        'state': 'opened',
+                        'squash': to_squash,
+                        'head': new_head,
+                    })
+                    _logger.warning(
+                        "head mismatch on %s: had %s but found %s",
+                        pr.display_name, old_head, new_head
+                    )
+                    msg = "We apparently missed an update to this PR and" \
+                          " tried to stage it in a state which might not have" \
+                          " been approved. PR has been updated to %s, please" \
+                          " check and approve or re-approve." % new_head
+                else:
+                    _logger.exception("Failed to merge %s into staging branch (error: %s)",
+                                      pr.display_name, e)
+                    pr.state = 'error'
+                    msg = "Unable to stage PR (%s)" % e
+
                 self.env['runbot_merge.pull_requests.feedback'].create({
                     'repository': pr.repository.id,
                     'pull_request': pr.number,
-                    'message': "Unable to stage PR (%s)" % e,
+                    'message': msg,
                 })
 
                 # reset the head which failed, as rebase() may have partially
@@ -1746,8 +1792,8 @@ class FetchJob(models.Model):
     _name = 'runbot_merge.fetch_job'
 
     active = fields.Boolean(default=True)
-    repository = fields.Many2one('runbot_merge.repository', index=True, required=True)
+    repository = fields.Many2one('runbot_merge.repository', required=True)
-    number = fields.Integer(index=True, required=True)
+    number = fields.Integer(required=True)
 
 # The commit (and PR) statuses was originally a map of ``{context:state}``
 # however it turns out to clarify error messages it'd be useful to have

runbot_merge/tests/conftest.py

@@ -14,13 +14,11 @@ def page(port):
         return r.content
     return get
 
-# env['runbot_merge.project']._check_fetch()
-# runbot_merge.fetch_prs_cron
 @pytest.fixture
 def default_crons():
     return [
         # env['runbot_merge.project']._check_fetch()
-        # 'runbot_merge.fetch_prs_cron',
+        'runbot_merge.fetch_prs_cron',
         # env['runbot_merge.commit']._notify()
         'runbot_merge.process_updated_commits',
         # env['runbot_merge.project']._check_progress()
@@ -39,4 +37,4 @@ def project(env, config):
         'github_prefix': 'hansen',
         'branch_ids': [(0, 0, {'name': 'master'})],
         'required_statuses': 'legal/cla,ci/runbot',
-    })
+    })

runbot_merge/tests/test_basic.py

@@ -1959,6 +1959,70 @@ class TestPRUpdate(object):
         assert pr.head == c2
         assert pr.state == 'validated'
 
+    def test_update_missed(self, env, repo, config):
+        """ Sometimes github's webhooks don't trigger properly, a branch's HEAD
+        does not get updated and we might e.g. attempt to merge a PR despite it
+        now being unreviewed or failing CI or somesuch.
+
+        This is not a super frequent occurrence, and possibly not the most
+        problematic issue ever (e.g. if the branch doesn't CI it's not going to
+        pass staging, though we might still be staging a branch which had been
+        unreviewed).
+
+        So during the staging process, the heads should be checked, and the PR
+        will not be staged if the heads don't match (though it'll be reset to
+        open, rather than put in an error state as technically there's no
+        failure, we just want to notify users that something went odd with the
+        mergebot).
+
+        TODO: other cases / situations where we want to update the head?
+        """
+        with repo:
+            repo.make_commits(None, repo.Commit('m', tree={'a': '0'}), ref='heads/master')
+
+            [c] = repo.make_commits(
+                'heads/master', repo.Commit('c', tree={'a': '1'}), ref='heads/abranch')
+            pr = repo.make_pr(target='master', head='abranch')
+            repo.post_status(pr.head, 'success', 'legal/cla')
+            repo.post_status(pr.head, 'success', 'ci/runbot')
+            pr.post_comment('hansen r+', config['role_reviewer']['token'])
+        pr_id = env['runbot_merge.pull_requests'].search([
+            ('repository.name', '=', repo.name),
+            ('number', '=', pr.number),
+        ])
+        env.run_crons('runbot_merge.process_updated_commits')
+        assert pr_id.state == 'ready'
+
+        # TODO: find way to somehow skip / ignore the update_ref?
+        with repo:
+            # can't push a second commit because then the staging crashes due
+            # to the PR *actually* having more than 1 commit and thus needing
+            # a configuration
+            [c2] = repo.make_commits('heads/master', repo.Commit('c2', tree={'a': '2'}))
+            repo.post_status(c2, 'success', 'legal/cla')
+            repo.post_status(c2, 'success', 'ci/runbot')
+            repo.update_ref(pr.ref, c2, force=True)
+
+        # we missed the update notification so the db should still be at c and
+        # in a "ready" state
+        pr_id.write({
+            'head': c,
+            'state': 'ready',
+        })
+
+        env.run_crons()
+
+        # the PR should not get merged, and should be updated
+        assert pr_id.state == 'validated'
+        assert pr_id.head == c2
+
+        pr_id.write({'head': c, 'state': 'ready'})
+        with repo:
+            pr.post_comment('hansen check')
+        env.run_crons()
+        assert pr_id.state == 'validated'
+        assert pr_id.head == c2
+
 class TestBatching(object):
     def _pr(self, repo, prefix, trees, *, target='master', user, reviewer,
             statuses=(('ci/runbot', 'success'), ('legal/cla', 'success'))