From 0dd11acce84bdbbafbdd190fc4fabcada671df80 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 11 Feb 2025 14:27:53 +0100 Subject: [PATCH] [FIX] *: double forwardport when adding a PR to an existing batch This is a bit of an odd case which was only noticed because of persistent forwardport.batches, which ended up having a ton of related traceback in the logs (the mergebot kept trying to create forward ports from Jan 27th to Feb 10th, thankfully the errors happened in git so did not seem to eat through our API rate limiting). The issue was triggered by the addition of odoo/enterprise#77876 to odoo/odoo#194818. This triggered a completion job which led to the creation of odoo/enterprise#77877 to odoo/enterprise#77880, so far so good. Except the bit of code responsible for creating completion jobs only checked if the PR was being added to a batch with a descendant. That is the case of odoo/enterprise#77877 to odoo/enterprise#77879 (not odoo/enterprise#77880 because that's the end of the line). As a result, those triggered 3 more completion jobs, which kept failing in a loop because they tried pushing different commits to their next-siblings (without forcing, leading git to reject the non-ff push, hurray). A completion request should only be triggered by the addition of a new *source* (a PR without a source) to an existing batch with descendants, so add that to the check. This requires updating `_from_json` to create PRs in a single step (rather than one step to create based on github's data, and an other one for the hierarchical tracking) as we need the source to be set during `create` not as a post-action. Although there was a test which could have triggered this issue, the test only had 3 branches so was not long enough to trigger the issue: - Initial PR 1 on branch A merged then forward-ported to B and C. - Sibling PR 2 added to the batch in B. - Completed to C. - Ending there as C(1) has no descendant batch, leading to no further completion request. Adding a 4th branch did surface / show the issue by providing space for a new completion request from the creation of C(2). Interestingly even though I the test harness attempts to run all triggered crons to completion there can be misses, so the test can fail in two different ways (being now checked for): - there's a leftover forwardport.batch after we've created all our forwardports - there's an extra PR targeting D, descending from C(2) - in almost every case there's also a traceback in the logs, which does fail the build thanks to the `env` fixture's check --- forwardport/models/forwardport.py | 20 +++---- forwardport/models/project.py | 7 ++- forwardport/tests/test_batches.py | 72 +++++++++++++----------- runbot_merge/models/backport/__init__.py | 14 ++--- runbot_merge/models/batch.py | 28 +++++---- runbot_merge/models/pull_requests.py | 3 +- 6 files changed, 75 insertions(+), 69 deletions(-) diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index a0e36ac8..d5e63dbb 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -241,17 +241,15 @@ class ForwardPortTasks(models.Model, Queue): _logger.warning("Deleting %s:%s=%s", remote_target, ref, d.text) raise RuntimeError(f"Forwardport failure: {pr.display_name} ({r.text})") - new_pr = PullRequests._from_gh(r.json()) - _logger.info("Created forward-port PR %s", new_pr) - new_pr.write({ - 'batch_id': descendant.id, # should already be set correctly but... - 'merge_method': pr.merge_method, - 'source_id': source.id, - # only link to previous PR of sequence if cherrypick passed - # FIXME: apply parenting of siblings? Apply parenting *to* siblings? - 'parent_id': pr.id if not conflict else False, - 'detach_reason': "{1}\n{2}".format(*conflict).strip() if conflict else None, - }) + new_pr = PullRequests._from_gh( + r.json(), + batch_id=descendant.id, + merge_method=pr.merge_method, + source_id=source.id, + parent_id=False if conflict else pr.id, + detach_reason="{1}\n{2}".format(*conflict).strip() if conflict else None + ) + _logger.info("Created forward-port PR %s", new_pr.display_name) if conflict: self.env.ref('runbot_merge.forwardport.failure.conflict')._send( diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 0e9aed09..ebb63e80 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -213,10 +213,11 @@ class PullRequests(models.Model): new = super().create(to_create) for pr in new: - # added a new PR to an already forward-ported batch: port the PR - if self.env['runbot_merge.batch'].search_count([ + # added a new PR to an already forward-ported batch: immediately + # port forward to complete the genealogy + if not pr.source_id and self.env['runbot_merge.batch'].search_count([ ('parent_id', '=', pr.batch_id.id), - ]): + ], limit=1): self.env['forwardport.batches'].create({ 'batch_id': pr.batch_id.id, 'source': 'complete', diff --git a/forwardport/tests/test_batches.py b/forwardport/tests/test_batches.py index 7a53bfcf..13e14e0b 100644 --- a/forwardport/tests/test_batches.py +++ b/forwardport/tests/test_batches.py @@ -227,6 +227,13 @@ def test_add_to_forward_ported(env, config, make_repo, users): # region setup r1, _ = make_basic(env, config, make_repo, statuses="default") r2, fork2 = make_basic(env, config, make_repo, statuses="default") + project = env['runbot_merge.project'].search([]) + project.write({ + 'branch_ids': [(0, 0, {'name': 'd', 'sequence': 40})], + }) + with r1, r2: + r1.make_commits('c', Commit('1111', tree={'g': 'a'}), ref='heads/d') + r2.make_commits('c', Commit('1111', tree={'g': 'a'}), ref='heads/d') with r1: r1.make_commits('a', Commit('a', tree={'a': 'a'}), ref="heads/pr1") @@ -241,59 +248,60 @@ def test_add_to_forward_ported(env, config, make_repo, users): # region port forward pr1_a_id = to_pr(env, pr1_a) - pr1_b_id = pr1_a_id.forwardport_ids - assert pr1_b_id - with r1: - r1.post_status(pr1_b_id.head, 'success') - env.run_crons() - pr1_c_id = pr1_a_id.forwardport_ids - pr1_b_id - assert pr1_c_id + for branch in 'bcd': + next_pr = env['runbot_merge.pull_requests'].search([ + ('source_id', '=', pr1_a_id.id), + ('target.name', '=', branch), + ]) + assert next_pr + with r1: + r1.post_status(next_pr.head, 'success') + env.run_crons() # endregion # endregion + pr1_b_id, pr1_c_id, pr1_d_id = pr1_a_id.forwardport_ids[::-1] # new PR must be in fork for labels to actually match with r2, fork2: # branch in fork has no owner prefix, but HEAD for cross-repo PR does fork2.make_commits(r2.commit("b").id, Commit('b', tree={'b': 'b'}), ref=f'heads/{pr1_b_id.refname}') pr2_b = r2.make_pr(title="b", target="b", head=pr1_b_id.label) - r2.post_status(pr2_b.head, 'success') env.run_crons() pr2_b_id = to_pr(env, pr2_b) assert pr2_b_id.batch_id == pr1_b_id.batch_id - assert len(pr2_b_id.forwardport_ids) == 1, \ + assert len(pr2_b_id.forwardport_ids) == 2, \ "since the batch is already forward ported, the new PR should" \ " immediately be forward ported to match" - assert pr2_b_id.forwardport_ids.label == pr1_c_id.label - pr2_a = r1.get_pr(pr1_b_id.number) + assert pr2_b_id.forwardport_ids.mapped('label') == (pr1_d_id | pr1_c_id).mapped('label') + + assert not (b := env['forwardport.batches'].search([])),\ + f"there should not be any more forward porting, found {b.read(['source', 'pr_id'])}" + known_set = pr1_a_id | pr1_a_id.forwardport_ids | pr2_b_id | pr2_b_id.forwardport_ids + assert not (p := (env['runbot_merge.pull_requests'].search([]) - known_set)), \ + "there should not be any PR outside of the sequences we created, "\ + f"found {p.read(['source_id', 'parent_id', 'display_name'])}" + + with r2: + for pr_id in pr2_b_id | pr2_b_id.forwardport_ids: + r2.post_status(pr_id.head, 'success') + with r1, r2: - pr2_a.post_comment('hansen r+', config['role_reviewer']['token']) - pr2_b.post_comment("hansen r+", config['role_reviewer']['token']) + r1.get_pr(pr1_a_id.forwardport_ids[0].number)\ + .post_comment('hansen r+', config['role_reviewer']['token']) + r2.get_pr(pr2_b_id.forwardport_ids[0].number) \ + .post_comment('hansen r+', config['role_reviewer']['token']) env.run_crons() with r1, r2: - r1.post_status('staging.b', 'success') - r2.post_status('staging.b', 'success') + for branch in 'bcd': + r1.post_status(f'staging.{branch}', 'success') + r2.post_status(f'staging.{branch}', 'success') env.run_crons() - assert pr1_b_id.state == 'merged' - assert pr2_b_id.state == 'merged' - - assert len(pr2_b_id.forwardport_ids) == 1,\ - "verify that pr2_b did not get forward ported again on merge" - pr2_c = r2.get_pr(pr2_b_id.forwardport_ids.number) - assert pr2_c.comments == [ - seen(env, pr2_c, users), - (users['user'], '''\ -@{user} this PR targets c and is the last of the forward-port chain. - -To merge the full chain, use -> @hansen r+ - -More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port -'''.format_map(users)), - ] + assert not env['runbot_merge.pull_requests'].search([]) - known_set, \ + "there should not be any PR outside of the sequences we created" def test_add_to_forward_port_conflict(env, config, make_repo, users): """If a PR is added to an existing forward port sequence, and it causes diff --git a/runbot_merge/models/backport/__init__.py b/runbot_merge/models/backport/__init__.py index 1812c222..d92a70f2 100644 --- a/runbot_merge/models/backport/__init__.py +++ b/runbot_merge/models/backport/__init__.py @@ -116,15 +116,15 @@ class PullRequestBackport(models.TransientModel): if not r.ok: raise UserError(f"Backport PR creation failure: {r.text}") - backport = self.env['runbot_merge.pull_requests']._from_gh(r.json()) - _logger.info("Created backport %s for %s", backport.display_name, self.pr_id.display_name) - - backport.write({ - 'merge_method': self.pr_id.merge_method, + backport = self.env['runbot_merge.pull_requests']._from_gh( + r.json(), + merge_method=self.pr_id.merge_method, # the backport's own forwardport should stop right before the # original PR by default - 'limit_id': branches[source_idx - 1], - }) + limit_id=branches[source_idx - 1], + ) + _logger.info("Created backport %s for %s", backport.display_name, self.pr_id.display_name) + self.env['runbot_merge.pull_requests.tagging'].create({ 'repository': repo_id.id, 'pull_request': backport.number, diff --git a/runbot_merge/models/batch.py b/runbot_merge/models/batch.py index 1480eaf0..458ba1a7 100644 --- a/runbot_merge/models/batch.py +++ b/runbot_merge/models/batch.py @@ -364,23 +364,21 @@ class Batch(models.Model): _logger.warning("Deleting %s:%s=%s", repo.fp_remote_target, new_branch, d.text) raise RuntimeError(f"Forwardport failure: {pr.display_name} ({r.text})") - new_pr = PRs._from_gh(r.json()) + report_conflicts = has_conflicts and self.fw_policy != 'skipmerge' + new_pr = PRs._from_gh( + r.json(), + merge_method=pr.merge_method, + source_id=source.id, + parent_id=False if report_conflicts else pr.id, + detach_reason=report_conflicts and "conflicts:\n{}".format( + '\n\n'.join( + f"{out}\n{err}".strip() + for _, out, err, _ in filter(None, conflicts.values()) + ) + ), + ) _logger.info("Created forward-port PR %s", new_pr) new_batch |= new_pr - - report_conflicts = has_conflicts and self.fw_policy != 'skipmerge' - - # allows PR author to close or skipci - new_pr.write({ - 'merge_method': pr.merge_method, - 'source_id': source.id, - # only link to previous PR of sequence if cherrypick passed - 'parent_id': pr.id if not report_conflicts else False, - 'detach_reason': "conflicts:\n{}".format('\n\n'.join( - f"{out}\n{err}".strip() - for _, out, err, _ in filter(None, conflicts.values()) - )) if report_conflicts else None, - }) if report_conflicts and pr.parent_id and pr.state not in ('merged', 'closed'): self.env.ref('runbot_merge.forwardport.failure.conflict')._send( repository=pr.repository, diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 081015a7..c9a092e1 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -1397,7 +1397,7 @@ For your own safety I've ignored *everything in your entire comment*. ) return prs - def _from_gh(self, description, author=None, branch=None, repo=None): + def _from_gh(self, description, author=None, branch=None, repo=None, **kwargs): if repo is None: repo = self.env['runbot_merge.repository'].search([ ('name', '=', description['base']['repo']['full_name']), @@ -1423,6 +1423,7 @@ For your own safety I've ignored *everything in your entire comment*. 'squash': description['commits'] == 1, 'message': utils.make_message(description), 'draft': description['draft'], + **kwargs, }) def write(self, vals):