[IMP] runbot_merge: fix dual merge of split prioritised PRs

Because `alone` (formerly p != 2) is selected before split PRs, if a
prioritised PR gets split (or a split PR gets prioritised) it will be
staged once as prioritised, and again because split.

Improve the selection of ready batches to exclude split batches
upstream, such that they don't have to be rechecked over and over, and
their priorities don't cause us issues.

runbot_merge/models/stagings_create.py

@@ -61,8 +61,8 @@ def try_staging(branch: Branch) -> Optional[Stagings]:
     def log(label: str, batches: Batch) -> None:
         _logger.info(label, ', '.join(batches.mapped('prs.display_name')))
 
-    alone, batches = ready_prs(for_branch=branch)
-    print('alone', alone, 'ready', batches)
+    alone, batches = ready_batches(for_branch=branch)
+
     if alone:
         log("staging high-priority PRs %s", batches)
     elif branch.project_id.staging_priority == 'default':
@@ -75,13 +75,6 @@ def try_staging(branch: Branch) -> Optional[Stagings]:
             # first as long as there's room
             log("staging ready PRs %s (prioritising splits)", batches)
     elif branch.project_id.staging_priority == 'ready':
-        # splits are ready by definition, we need to exclude them from the
-        # ready rows otherwise we'll re-stage the splits so if an error is legit
-        # we cycle forever
-        # FIXME: once the batches are less shit, store this durably on the
-        #        batches and filter out when fetching readies (?)
-        batches -= branch.split_ids.batch_ids
-
         if batches:
             log("staging ready PRs %s (prioritising ready)", batches)
         else:
@@ -91,11 +84,6 @@ def try_staging(branch: Branch) -> Optional[Stagings]:
             log("staging split PRs %s (prioritising ready)", batches)
     else:
         assert branch.project_id.staging_priority == 'largest'
-        # splits are ready by definition, we need to exclude them from the
-        # ready rows otherwise ready always wins but we re-stage the splits, so
-        # if an error is legit we'll cycle forever
-        batches -= branch.split_ids.batch_ids
-
         maxsplit = max(branch.split_ids, key=lambda s: len(s.batch_ids), default=branch.env['runbot_merge.split'])
         _logger.info("largest split = %d, ready = %d", len(maxsplit.batch_ids), len(batches))
         # bias towards splits if len(ready) = len(batch_ids)
@@ -201,13 +189,17 @@ For-Commit-Id: {it.head}
     return st
 
 
-def ready_prs(for_branch: Branch) -> Tuple[bool, Batch]:
+def ready_batches(for_branch: Branch) -> Tuple[bool, Batch]:
     env = for_branch.env
+    # splits are ready by definition, we need to exclude them from the ready
+    # rows otherwise if a prioritised (alone) PR is part of a split it'll be
+    # staged through priority *and* through split.
+    split_ids = for_branch.split_ids.batch_ids.ids
     env.cr.execute("""
         SELECT max(priority)
         FROM runbot_merge_batch
-        WHERE blocked IS NULL AND target = %s
-    """, [for_branch.id])
+        WHERE blocked IS NULL AND target = %s AND NOT id = any(%s)
+    """, [for_branch.id, split_ids])
     alone = env.cr.fetchone()[0] == 'alone'
 
     return (
@@ -216,6 +208,7 @@ def ready_prs(for_branch: Branch) -> Tuple[bool, Batch]:
             ('target', '=', for_branch.id),
             ('blocked', '=', False),
             ('priority', '=', 'alone') if alone else (1, '=', 1),
+            ('id', 'not in', split_ids),
         ], order="priority DESC, id ASC"),
     )
 

runbot_merge/tests/test_basic.py

@@ -2969,6 +2969,50 @@ class TestBatching(object):
         assert not p_01.staging_id, "p_01 should not be picked up as it's failed"
         assert p_21.staging_id, "p_21 should have been staged"
 
+    def test_urgent_split(self, env, repo, config):
+        """Ensure that urgent (alone) PRs which get split don't get
+        double-merged
+        """
+        with repo:
+            repo.make_commits(
+                None,
+                Commit("initial", tree={'a': '1'}),
+                ref="heads/master"
+            )
+
+            pr01 = self._pr(
+                repo, "PR1", [{'b': '1'}],
+                user=config['role_user']['token'],
+                reviewer=None,
+            )
+            pr01.post_comment('hansen alone r+', config['role_reviewer']['token'])
+            pr02 = self._pr(
+                repo, "PR2", [{'c': '1'}],
+                user=config['role_user']['token'],
+                reviewer=None,
+            )
+            pr02.post_comment('hansen alone r+', config['role_reviewer']['token'])
+        env.run_crons('runbot_merge.process_updated_commits')
+        pr01_id = to_pr(env, pr01)
+        assert pr01_id.blocked is False
+        pr02_id = to_pr(env, pr02)
+        assert pr01_id.blocked is False
+
+        env.run_crons()
+        st = pr01_id.staging_id
+        assert st and pr02_id.staging_id == st
+        with repo:
+            repo.post_status('staging.master', 'failure', 'ci/runbot')
+        env.run_crons()
+        # should have cancelled the staging, split it, and re-staged the first
+        # half of the split
+        assert st.state == 'failure'
+        assert pr01_id.staging_id and pr01_id.staging_id != st
+        assert not pr02_id.staging_id
+        split_prs = env['runbot_merge.split'].search([]).batch_ids.prs
+        assert split_prs == pr02_id, \
+            f"only the unstaged PR {pr02_id} should be in a split, found {split_prs}"
+
     @pytest.mark.skip(reason="Maybe nothing to do, the PR is just skipped and put in error?")
     def test_batching_merge_failure(self):
         pass