[FIX] runbot_merge: when fetching a PR, sync closed state

If a PR is closed on github and unknown by the mergebot, when fetched
it should be properly sync'd as "closed" in the backend, otherwise the
PR can get in a weird state and cause issues.

Also move the "I fetched the thing" comment before the actual creation
of the PR for workflow clarity, otherwise the reader has the
impression that the 'bot knew about the PR then fetched it anyway.

And improve savepoint management around the fetching: savepoints
should be released in all cases.

Closes #488.

runbot_merge/controllers/__init__.py

@@ -193,16 +193,22 @@ def handle_pr(env, event):
 
     # don't marked merged PRs as closed (!!!)
     if event['action'] == 'closed' and pr_obj.state != 'merged':
-        # FIXME: store some sort of "try to close it later" if the merge fails?
-        _logger.info(
-            '%s closing %s (state=%s)',
-            event['sender']['login'],
-            pr_obj.display_name,
-            pr_obj.state,
-        )
+        oldstate = pr_obj.state
         if pr_obj._try_closing(event['sender']['login']):
+            _logger.info(
+                '%s closed %s (state=%s)',
+                event['sender']['login'],
+                pr_obj.display_name,
+                oldstate,
+            )
             return 'Closed {}'.format(pr_obj.id)
         else:
+            _logger.warning(
+                '%s tried to close %s (state=%s)',
+                event['sender']['login'],
+                pr_obj.display_name,
+                oldstate,
+            )
             return 'Ignored: could not lock rows (probably being merged)'
 
     if event['action'] == 'reopened' :

runbot_merge/models/pull_requests.py

@@ -210,7 +210,8 @@ class Project(models.Model):
             except Exception:
                 self.env.cr.execute("ROLLBACK TO SAVEPOINT runbot_merge_before_fetch")
                 _logger.exception("Failed to load pr %s, skipping it", f.number)
-            self.env.cr.execute("RELEASE SAVEPOINT runbot_merge_before_fetch")
+            finally:
+                self.env.cr.execute("RELEASE SAVEPOINT runbot_merge_before_fetch")
 
             # commit after each fetched PR
             f.active = False
@@ -329,6 +330,7 @@ All substitutions are tentatively applied sequentially to the input.
             r = controllers.handle_pr(self.env, {
                 'action': 'synchronize',
                 'pull_request': pr,
+                'sender': {'login': self.project_id.github_prefix}
             })
             feedback({
                 'repository': pr_id.repository.id,
@@ -337,11 +339,21 @@ All substitutions are tentatively applied sequentially to the input.
             })
             return
 
+        feedback({
+            'repository': self.id,
+            'pull_request': number,
+            'message': "Sorry, I didn't know about this PR and had to retrieve "
+                       "its information, you may have to re-approve it."
+        })
         # init the PR to the null commit so we can later synchronise it back
         # back to the "proper" head while resetting reviews
         controllers.handle_pr(self.env, {
             'action': 'opened',
-            'pull_request': {**pr, 'head': {**pr['head'], 'sha': '0'*40}},
+            'pull_request': {
+                **pr,
+                'head': {**pr['head'], 'sha': '0'*40},
+                'state': 'open',
+            },
         })
         # fetch & set up actual head
         for st in gh.statuses(pr['head']['sha']):
@@ -378,12 +390,14 @@ All substitutions are tentatively applied sequentially to the input.
             'pull_request': pr,
             'sender': {'login': self.project_id.github_prefix}
         })
-        feedback({
-            'repository': self.id,
-            'pull_request': number,
-            'message': "Sorry, I didn't know about this PR and had to retrieve "
-                       "its information, you may have to re-approve it."
-        })
+        if pr['state'] == 'closed':
+            # don't go through controller because try_closing does weird things
+            # for safety / race condition reasons which ends up committing
+            # and breaks everything
+            self.env['runbot_merge.pull_requests'].search([
+                ('repository.name', '=', pr['base']['repo']['full_name']),
+                ('number', '=', number),
+            ]).state = 'closed'
 
     def having_branch(self, branch):
         branches = self.env['runbot_merge.branch'].search
@@ -1214,6 +1228,7 @@ class PullRequests(models.Model):
         if body:
             message += '\n\n' + body
         return self.env['runbot_merge.pull_requests'].create({
+            'state': 'opened' if description['state'] == 'open' else 'closed',
             'number': description['number'],
             'label': repo._remap_label(description['head']['label']),
             'author': author.id,

runbot_merge/tests/test_basic.py

@@ -2894,7 +2894,6 @@ class TestUnknownPR:
         assert not Fetch.search([('repository', '=', repo.name), ('number', '=', prx.number)])
 
         c = env['runbot_merge.commit'].search([('sha', '=', prx.head)])
-        print(prx.head, c, c.statuses, flush=True)
         assert json.loads(c.statuses) == {
             'legal/cla': {'state': 'success', 'target_url': None, 'description': None},
             'ci/runbot': {'state': 'success', 'target_url': 'http://example.org/wheee', 'description': None}
@@ -2903,10 +2902,10 @@ class TestUnknownPR:
             seen(env, prx, users),
             (users['reviewer'], 'hansen r+'),
             (users['reviewer'], 'hansen r+'),
-            seen(env, prx, users),
             (users['user'], "Sorry, I didn't know about this PR and had to "
                             "retrieve its information, you may have to "
                             "re-approve it."),
+            seen(env, prx, users),
         ]
 
         pr = env['runbot_merge.pull_requests'].search([
@@ -2919,6 +2918,49 @@ class TestUnknownPR:
             prx.post_comment('hansen r+', config['role_reviewer']['token'])
         assert pr.state == 'ready'
 
+    def test_fetch_closed(self, env, repo, users, config):
+        """ If an "unknown PR" is fetched while closed, it should be saved as
+        closed
+        """
+        with repo:
+            m, _ = repo.make_commits(
+                None,
+                Commit('initial', tree={'m': 'm'}),
+                Commit('second', tree={'m2': 'm2'}),
+                ref='heads/master')
+
+            [c1] = repo.make_commits(m, Commit('first', tree={'m': 'c1'}))
+            pr = repo.make_pr(title='title', body='body', target='master', head=c1)
+        env.run_crons()
+        with repo:
+            pr.close()
+
+        # assume an unknown but ready PR: we don't know the PR or its head commit
+        to_pr(env, pr).unlink()
+        env['runbot_merge.commit'].search([('sha', '=', pr.head)]).unlink()
+
+        # reviewer reviewers
+        with repo:
+            pr.post_comment('hansen r+', config['role_reviewer']['token'])
+
+        Fetch = env['runbot_merge.fetch_job']
+        fetches = Fetch.search([('repository', '=', repo.name), ('number', '=', pr.number)])
+        assert len(fetches) == 1, f"expected one fetch for {pr.number}, found {len(fetches)}"
+
+        env.run_crons('runbot_merge.fetch_prs_cron')
+        env.run_crons()
+        assert not Fetch.search([('repository', '=', repo.name), ('number', '=', pr.number)])
+
+        assert to_pr(env, pr).state == 'closed'
+        assert pr.comments == [
+            seen(env, pr, users),
+            (users['reviewer'], 'hansen r+'),
+            (users['user'], "Sorry, I didn't know about this PR and had to "
+                            "retrieve its information, you may have to "
+                            "re-approve it."),
+            seen(env, pr, users),
+        ]
+
     def test_rplus_unmanaged(self, env, repo, users, config):
         """ r+ on an unmanaged target should notify about
         """

runbot_merge/tests/test_multirepo.py

@@ -896,6 +896,7 @@ class TestSubstitutions:
                         'full_name': r.name,
                     },
                     'pull_request': {
+                        'state': 'open',
                         'user': {'login': 'bob'},
                         'base': {
                             'repo': {'full_name': r.name},