[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

@@ -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(

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)