[FIX] runbot_merge: handling of staging cancellation

Move staging cancellation to the batch, remove its (complicated)
handling from the PRs.

This loses some precision in the cancellation message, but that could
likely be recovered (in part) by adding more precise checks &
diagnostic extractions in the compute.

conftest.py

@@ -1175,6 +1175,9 @@ class Model:
     def __len__(self):
         return len(self._ids)
 
+    def __hash__(self):
+        return hash((self._model, frozenset(self._ids)))
+
     def __eq__(self, other):
         if not isinstance(other, Model):
             return NotImplemented

runbot_merge/models/batch.py

@@ -92,4 +92,11 @@ class Batch(models.Model):
                     p.display_name for p in unready
                 )
             else:
+                if batch.blocked and batch.cancel_staging:
+                    batch.target.active_staging_id.cancel(
+                        'unstaged by %s on %s (%s)',
+                        self.env.user.login,
+                        batch,
+                        ', '.join(batch.prs.mapped('display_name')),
+                    )
                 batch.blocked = False

runbot_merge/models/commands.py

@@ -281,6 +281,8 @@ class Parser:
         return Priority.ALONE
 
     def parse_cancel(self) -> CancelStaging:
+        self.assert_next('=')
+        self.assert_next('staging')
         return CancelStaging()
 
     def parse_skipchecks(self) -> SkipChecks:

runbot_merge/models/pull_requests.py

@@ -700,7 +700,7 @@ class PullRequests(models.Model):
                     else:
                         msg = self._approve(author, login)
                 case commands.Reject() if is_author:
-                    if cancellers := self.batch_id.cancel_staging:
+                    if self.batch_id.cancel_staging:
                         self.batch_id.cancel_staging = False
                     if self.batch_id.skipchecks or self.reviewed_by:
                         if self.error:
@@ -756,14 +756,11 @@ class PullRequests(models.Model):
                             p.reviewed_by = author
                 case commands.CancelStaging() if is_admin:
                     self.batch_id.cancel_staging = True
-                    # FIXME: remove this when skipchecks properly affects state,
-                    #        maybe: staging cancellation should then only occur
-                    #        when a cancel_staging PR transitions to ready, or
-                    #        a ready PR is flagged as cancelling staging
-                    self.target.active_staging_id.cancel(
-                        "Unstaged by %s on %s",
-                        author.github_login, self.display_name,
-                    )
+                    if not self.batch_id.blocked:
+                        self.target.active_staging_id.cancel(
+                            "Unstaged by %s on %s",
+                            author.github_login, self.display_name,
+                        )
                 case commands.Override(statuses):
                     for status in statuses:
                         overridable = author.override_rights\
@@ -1081,12 +1078,6 @@ class PullRequests(models.Model):
     def write(self, vals):
         if vals.get('squash'):
             vals['merge_method'] = False
-        fields = []
-        canceler = vals.get('cancel_staging') or any(p.cancel_staging for p in self)
-        if canceler:
-            fields.append('state')
-            fields.append('skipchecks')
-        prev = {pr.id: {field: pr[field] for field in fields} for pr in self}
 
         # when explicitly marking a PR as ready
         if vals.get('state') == 'ready':
@@ -1144,21 +1135,6 @@ class PullRequests(models.Model):
         if newhead:
             c = self.env['runbot_merge.commit'].search([('sha', '=', newhead)])
             self._validate(c.statuses or '{}')
-
-        if canceler:
-            for pr in self:
-                if not pr.cancel_staging:
-                    continue
-
-                old = prev[pr.id]
-                def ready(pr):
-                    return pr['state'] == 'ready'\
-                       or (pr['skipchecks'] and pr['state'] != 'error')
-                if ready(pr) and not ready(old):
-                    if old['state'] == 'error': # error -> ready gets a bespoke message
-                        pr.target.active_staging_id.cancel(f"retrying {pr.display_name}")
-                    else:
-                        pr.target.active_staging_id.cancel(f"{pr.display_name} became ready")
         return w
 
     def _check_linked_prs_statuses(self, commit=False):

runbot_merge/tests/test_multirepo.py

@@ -9,6 +9,7 @@ import functools
 import operator
 import time
 import xmlrpc.client
+from itertools import repeat
 
 import pytest
 import requests
@@ -1468,3 +1469,65 @@ def test_freeze_conflict(env, project, repo_a, repo_b, repo_c, users, config):
     with pytest.raises(AssertionError) as e:
         repo_b.get_ref('heads/1.1')
     assert e.value.args[0].startswith("Not Found")
+
+def test_cancel_staging(env, project, repo_a, repo_b, users, config):
+    """If a batch is flagged as staging cancelling (from any PR), the staging
+    should get cancelled if and when the batch transitions to unblocked
+    """
+    with repo_a, repo_b:
+        make_branch(repo_a, 'master', 'initial', {'a': '1'})
+        make_branch(repo_b, 'master', 'initial', {'b': '1'})
+
+        pr_a = make_pr(repo_a, 'batch', [{'a': '2'}], user=config['role_user']['token'], statuses=[], reviewer=None)
+        pr_b = make_pr(repo_b, 'batch', [{'b': '2'}], user=config['role_user']['token'], statuses=[], reviewer=None)
+        pr_lone = make_pr(
+            repo_a,
+            "C",
+            [{'c': '1'}],
+            user=config['role_user']['token'],
+            reviewer=config['role_reviewer']['token'],
+        )
+    env.run_crons()
+
+    a_id, b_id, lone_id = map(to_pr, repeat(env), [pr_a, pr_b, pr_lone])
+    assert lone_id.staging_id
+    st = lone_id.staging_id
+
+    with repo_a:
+        pr_a.post_comment("hansen cancel=staging", config['role_reviewer']['token'])
+    assert a_id.state == 'opened'
+    assert a_id.cancel_staging
+    assert b_id.cancel_staging
+    assert lone_id.staging_id == st
+    with repo_a:
+        pr_a.post_comment('hansen r+', config['role_reviewer']['token'])
+    assert a_id.state == 'approved'
+    assert lone_id.staging_id == st
+    with repo_a:
+        repo_a.post_status(a_id.head, 'success', 'ci/runbot')
+        repo_a.post_status(a_id.head, 'success', 'legal/cla')
+    env.run_crons()
+    assert a_id.state == 'ready'
+    assert lone_id.staging_id == st
+
+    assert b_id.state == 'opened'
+    with repo_b:
+        pr_b.post_comment('hansen r+', config['role_reviewer']['token'])
+    assert b_id.state == 'approved'
+    assert lone_id.staging_id == st
+    with repo_b:
+        repo_b.post_status(b_id.head, 'success', 'ci/runbot')
+        repo_b.post_status(b_id.head, 'success', 'legal/cla')
+    assert b_id.state == 'approved'
+    assert lone_id.staging_id == st
+    env.run_crons()
+    assert b_id.state == 'ready'
+    # should have cancelled the staging, picked a and b, and re-staged the
+    # entire thing
+    assert lone_id.staging_id != st
+
+    assert len({
+        lone_id.staging_id.id,
+        a_id.staging_id.id,
+        b_id.staging_id.id,
+    }) == 1