From c6755a045afe95efbef7aff9952a1762966ab693 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 19 Oct 2021 10:30:55 +0200 Subject: [PATCH] [FIX] forwardport: possible race in forwardport followup update Normally when a forwardport is updated the forward-bot cascades the update onto its followups (if it has any), but takes care to keep the followups attached as they were not updated "by hand". In the case of odoo/odoo#77677 however that did not work and the followup PRs got detached. Looking at the logs, it becomes flagrant that there was a race condition: either Git took a long time to respond to the push, or there was an IO slowdown which led to the "local update" taking a very long time. Either way this allowed the "synchronize" webhook from github to arrive before the current transaction was committed, rolling back said transaction and making the forwardbot assume this was a "real" sync and detach the followup from its parent. Locking the PR row up-front ought fix the issue, and also move the local update to before having pushed: the "extra" commits in the local cache don't matter too much even if pushing to github fails, they'll be cleaned up by a GC eventually. Also migrate the `-f` on push to `--force-if-includes` in order to avoid possible race conditions on the push (since we're not fetching the current branch, use the full-syntax explicit CAS form, that's exactly what we're looking for anyway). Fixes #541 (hopefully) --- forwardport/models/forwardport.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index a8cc503d..9440c389 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -102,6 +102,12 @@ class UpdateQueue(models.Model, Queue): previous = self.new_root with ExitStack() as s: for child in self.new_root._iter_descendants(): + self.env.cr.execute(""" + SELECT id + FROM runbot_merge_pull_requests + WHERE id = %s + FOR UPDATE NOWAIT + """, [child.id]) _logger.info( "Re-port %s from %s (changed root %s -> %s)", child.display_name, @@ -149,22 +155,23 @@ class UpdateQueue(models.Model, Queue): f'{child.target.name}..{child.refname}', count=True ).stdout.decode().strip()) + old_head = child.head # update child's head to the head we're going to push child.with_context(ignore_head_update=True).write({ 'head': new_head, # 'state': 'opened', 'squash': commits_count == 1, }) - working_copy.push('-f', 'target', child.refname) - - # also push to local cache: looks like in some cases github - # doesn't propagate revisions (?) or at least does so too slowly - # so on the next loop we try to fetch the revision we just - # pushed through PR and... we can't find it + # push the new head to the local cache: in some cases github + # doesn't propagate revisions fast enough so on the next loop we + # can't find the revision we just pushed dummy_branch = str(uuid.uuid4()) ref = previous._get_local_directory() working_copy.push(ref._directory, f'{new_head}:refs/heads/{dummy_branch}') ref.branch('--delete', '--force', dummy_branch) + # then update the child's branch to the new head + working_copy.push(f'--force-with-lease={child.refname}:{old_head}', + 'target', child.refname) # committing here means github could technically trigger its # webhook before sending a response, but committing before