[FIX] runbot_merge: lock-in statuses after a staging has finished

The `statuses` field of a staging is always "live" because it's a
computed non-stored field. This is an issue when a staging finishes in
whatever state, then someone gets new statuses sent on one of the head
commits, either by rebuilding (part of) the staging or by just using
the same commit for one of their branches.

This makes the reporting of the main dashboard confusing, as one might
look at a failed staging and see all the required statuses
successful. It also makes post-mortem analysis more complicated as the
logs have to be trawled for what the statuses used to be (and they
don't always tell).

Solve this by storing a snapshot of the statuses the first time a
staging moves away from `pending`, whether it's to success or failure.

Fixes #667

conftest.py

@@ -783,7 +783,8 @@ class Repo:
     def post_status(self, ref, status, context='default', **kw):
         assert self.hook
         assert status in ('error', 'failure', 'pending', 'success')
-        r = self._session.post('https://api.github.com/repos/{}/statuses/{}'.format(self.name, self.commit(ref).id), json={
+        commit = ref if isinstance(ref, Commit) else self.commit(ref)
+        r = self._session.post('https://api.github.com/repos/{}/statuses/{}'.format(self.name, commit.id), json={
             'state': status,
             'context': context,
             **kw

runbot_merge/changelog/2022-10/statuses.md

@@ -0,0 +1,10 @@
+FIX: lock in statuses at the end of a staging
+
+The statuses of a staging are computed dynamically. Because github associates
+statuses with *commits*, rebuilding a staging (partially or completely) or using
+one of its commits for a branch could lead to the statuses becoming inconsistent
+with the staging e.g. all-green statuses while the staging had failed.
+
+By locking in the status at the end of the staging, the dashboard is less 
+confusing and more consistent, and post-mortem analysis (e.g. of staging
+failures) easier.

runbot_merge/models/pull_requests.py

@@ -1714,6 +1714,25 @@ class Stagings(models.Model):
     head_ids = fields.Many2many('runbot_merge.commit', compute='_compute_statuses')
 
     statuses = fields.Binary(compute='_compute_statuses')
+    statuses_cache = fields.Text()
+
+    def write(self, vals):
+        # don't allow updating the statuses_cache
+        vals.pop('statuses_cache', None)
+
+        if 'state' not in vals:
+            return super().write(vals)
+
+        previously_pending = self.filtered(lambda s: s.state == 'pending')
+        super(Stagings, self).write(vals)
+        for staging in previously_pending:
+            if staging.state != 'pending':
+                super(Stagings, staging).write({
+                    'statuses_cache': json.dumps(staging.statuses)
+                })
+
+        return True
+
 
     def name_get(self):
         return [
@@ -1738,6 +1757,10 @@ class Stagings(models.Model):
                 if not repo.endswith('^')
             }
             commits = st.head_ids = Commits.search([('sha', 'in', list(heads.keys()))])
+            if st.statuses_cache:
+                st.statuses = json.loads(st.statuses_cache)
+                continue
+
             st.statuses = [
                 (
                     heads[commit.sha],

runbot_merge/tests/test_oddities.py

@@ -1,6 +1,6 @@
 import requests
 
-from utils import Commit, to_pr
+from utils import Commit, to_pr, seen
 
 
 def test_partner_merge(env):
@@ -129,3 +129,51 @@ def test_unreviewer(env, project, port):
     assert 'error' not in r.json()
 
     assert p.review_rights == env['res.partner.review']
+
+def test_staging_post_update(env, project, make_repo, setreviewers, users, config):
+    """Because statuses come from commits, it's possible to update the commits
+    of a staging after that staging has completed (one way or the other), either
+    by sending statuses directly (e.g. rebuilding, for non-deterministic errors)
+    or just using the staging's head commit in a branch.
+
+    This makes post-mortem analysis quite confusing, so stagings should
+    "lock in" their statuses once they complete.
+    """
+    repo = make_repo('repo')
+    project.write({'repo_ids': [(0, 0, {
+        'name': repo.name,
+        'group_id': False,
+        'required_statuses': 'legal/cla,ci/runbot'
+    })]})
+    setreviewers(*project.repo_ids)
+
+    with repo:
+        [m] = repo.make_commits(None, Commit('initial', tree={'m': 'm'}), ref='heads/master')
+
+        repo.make_commits(m, Commit('thing', tree={'m': 'c'}), ref='heads/other')
+        pr = repo.make_pr(target='master', head='other')
+        repo.post_status(pr.head, 'success', 'ci/runbot')
+        repo.post_status(pr.head, 'success', 'legal/cla')
+        pr.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token'])
+    env.run_crons()
+    pr_id = to_pr(env, pr)
+    staging_id = pr_id.staging_id
+    assert staging_id
+
+    staging_head = repo.commit('staging.master')
+    with repo:
+        repo.post_status(staging_head, 'failure', 'ci/runbot')
+    env.run_crons()
+    assert pr_id.state == 'error'
+    assert staging_id.state == 'failure'
+    assert staging_id.statuses == [
+        [repo.name, 'ci/runbot', 'failure', ''],
+    ]
+
+    with repo:
+        repo.post_status(staging_head, 'success', 'ci/runbot')
+    env.run_crons()
+    assert staging_id.state == 'failure'
+    assert staging_id.statuses == [
+        [repo.name, 'ci/runbot', 'failure', ''],
+    ]