From ad1d590d9ca8c87c58bc1cf5c9c945dbad3cbdd3 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 8 Feb 2024 12:02:34 +0100 Subject: [PATCH] [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 | 27 ++++++---------- runbot_merge/tests/test_basic.py | 44 ++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/runbot_merge/models/stagings_create.py b/runbot_merge/models/stagings_create.py index 231ee969..017b63a9 100644 --- a/runbot_merge/models/stagings_create.py +++ b/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"), ) diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 273c0aae..dc458b34 100644 --- a/runbot_merge/tests/test_basic.py +++ b/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