From 2337bd85186de000d074e5a7178cfeb110d15de7 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 10 Feb 2022 13:42:32 +0100 Subject: [PATCH] [FIX] forwardport: chain crash on insert in forward-port cron On two of the freezes, thereafter the logs showed serial crashes in the forwardport cron when trying to find the insertion point for a new forward-port. The first time was not really diagnosed, the second time the cause was found to be a retargeted PR which led to a failure of the "insertion" forward port, which did not take that possibility in account (it assumed -- sensibly I believed -- that an intermediate FP following a branch insertion would always succeed, sadly the malevolent universe had other plans). So only insert the new forward port inside its sequence (if necessary) if the forward port actually succeeded, otherwise ignore it. Fixes #551 --- forwardport/models/forwardport.py | 23 +++++---- forwardport/tests/test_weird.py | 82 +++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 12 deletions(-) diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index e8eca387..9519e8ef 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -49,19 +49,7 @@ class ForwardPortTasks(models.Model, Queue): def _process_item(self): batch = self.batch_id - newbatch = batch.prs._port_forward() - # insert new barch in ancestry sequence unless conflict (= no parent) - if self.source == 'insert': - for pr in newbatch.prs: - if not pr.parent_id: - break - newchild = pr.search([ - ('parent_id', '=', pr.parent_id.id), - ('id', '!=', pr.id), - ]) - if newchild: - newchild.parent_id = pr.id if newbatch: _logger.info( @@ -70,6 +58,17 @@ class ForwardPortTasks(models.Model, Queue): batch, batch.prs, newbatch, newbatch.prs, ) + # insert new batch in ancestry sequence unless conflict (= no parent) + if self.source == 'insert': + for pr in newbatch.prs: + if not pr.parent_id: + break + newchild = pr.search([ + ('parent_id', '=', pr.parent_id.id), + ('id', '!=', pr.id), + ]) + if newchild: + newchild.parent_id = pr.id else: # reached end of seq (or batch is empty) # FIXME: or configuration is fucky so doesn't want to FP (maybe should error and retry?) _logger.info( diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index 31c9e777..b569f66d 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -659,6 +659,88 @@ def test_skip_ci_next(env, config, make_repo): assert pr1_id.state == 'opened' assert pr2_id.state == 'opened' +def test_retarget_after_freeze(env, config, make_repo, users): + """Turns out it was possible to trip the forwardbot if you're a bit of a + dick: the forward port cron was not resilient to forward port failure in + case of filling in new branches (forward ports existing across a branch + insertion so the fwbot would have to "fill in" for the new branch). + + But it turns out causing such failure is possible by e.g. regargeting the + latter port. In that case the reinsertion task should just do nothing, and + the retargeted PR should be forward-ported normally once merged. + """ + project, prod, _ = make_basic(env, config, make_repo, fp_token=True, fp_remote=True) + with prod: + [c] = prod.make_commits('b', Commit('thing', tree={'x': '1'}), ref='heads/mypr') + pr = prod.make_pr(target='b', head='mypr') + prod.post_status(c, 'success', 'ci/runbot') + prod.post_status(c, 'success', 'legal/cla') + pr.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + + original_pr_id = to_pr(env, pr) + assert original_pr_id.state == 'ready' + assert original_pr_id.staging_id + + with prod: + prod.post_status('staging.b', 'success', 'ci/runbot') + prod.post_status('staging.b', 'success', 'legal/cla') + env.run_crons() + # should have created a pr targeted to C + port_id = env['runbot_merge.pull_requests'].search([('state', 'not in', ('merged', 'closed'))]) + assert len(port_id) == 1 + assert port_id.target.name == 'c' + assert port_id.source_id == original_pr_id + assert port_id.parent_id == original_pr_id + + # because the module doesn't update the ordering of `branch_ids` to take + # `fp_sequence` in account so it's misleading + branch_c, branch_b, branch_a = branches_before = project.branch_ids.sorted('fp_sequence') + assert [branch_a.name, branch_b.name, branch_c.name] == ['a', 'b', 'c'] + # create branch so cron runs correctly + with prod: prod.make_ref('heads/bprime', prod.get_ref('c')) + project.write({ + 'branch_ids': [ + (1, branch_c.id, {'sequence': 1, 'fp_sequence': 20}), + (0, 0, {'name': 'bprime', 'sequence': 2, 'fp_sequence': 20, 'fp_target': True}), + (1, branch_b.id, {'sequence': 3, 'fp_sequence': 20}), + (1, branch_a.id, {'sequence': 4, 'fp_sequence': 20}), + ] + }) + new_branch = project.branch_ids - branches_before + assert new_branch.name == 'bprime' + + # should have added a job for the new fp + job = env['forwardport.batches'].search([]) + assert job + + # fuck up yo life: retarget the existing FP PR to the new branch + port_id.target = new_branch.id + + env.run_crons('forwardport.port_forward') + assert not job.exists(), "job should have succeeded and apoptosed" + + # since the PR was "already forward-ported" to the new branch it should not + # be touched + assert env['runbot_merge.pull_requests'].search([('state', 'not in', ('merged', 'closed'))]) == port_id + + # merge the retargered PR + port_pr = prod.get_pr(port_id.number) + with prod: + prod.post_status(port_pr.head, 'success', 'ci/runbot') + prod.post_status(port_pr.head, 'success', 'legal/cla') + port_pr.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + with prod: + prod.post_status('staging.bprime', 'success', 'ci/runbot') + prod.post_status('staging.bprime', 'success', 'legal/cla') + env.run_crons() + + new_pr_id = env['runbot_merge.pull_requests'].search([('state', 'not in', ('merged', 'closed'))]) + assert len(new_pr_id) == 1 + assert new_pr_id.parent_id == port_id + assert new_pr_id.target == branch_c + def test_approve_draft(env, config, make_repo, users): _, prod, _ = make_basic(env, config, make_repo, fp_token=True, fp_remote=True)