From 49c1d960fa36b543395a9106aac9e0c77e63be30 Mon Sep 17 00:00:00 2001 From: xmo-odoo Date: Tue, 7 Apr 2020 15:02:39 +0200 Subject: [PATCH] [FIX] forwardport: race condition on PR creation (#357) e9e08fec3c2a64a74e7171b4a8ee5f989f1d8733 attempted to fix the issue but obviously failed as it still occurs: when creating a PR through the API, it's possible that the webhook gets triggered fast enough the transaction creating the PR from the webhook commits before we get around to creating our own PR from the API call. In which case the forward port process aborts. The process is re-run later on and generally succeeds fully, but we're left with a dangling PR we created but couldn't do anything with as its use broke. This issue seems to be getting more frequent so it's becoming quite important to fix it. Therefore we give our Raging Bull a Big Gun and now he has 20 attack *cough cough* we lock the bloody table down tight (only allow concurrent `SELECT`) until we've got the PR back and we've done the updates we need to it and nobody can mess with it... probably. This is not ideal as it's going to block updates to completely unrelated PRs but it doesn't seem like postgres really allows for locking out creations without locking out the rest, short of using advisory locks maybe? E.g. in the `create` override get a `pg_advisory_xact_lock_shared`, then get a `pg_advisory_xact_lock` in the forward-port process that way we're just blocking the concurrent creation of PRs during forward port, but creations don't block one another and we don't block updates. Application-level locks wouldn't really work as the 'bot could be deployed using multi-worker scenarios so we'd need cross-process locks or something. Hopefully fixes #352 --- forwardport/models/project.py | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index e119c68e..baf8be1b 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -594,6 +594,7 @@ class PullRequests(models.Model): if not body: body = None + self.env.cr.execute('LOCK runbot_merge_pull_requests IN SHARE MODE') r = gh.post('https://api.github.com/repos/{}/pulls'.format(pr.repository.name), json={ 'base': target.name, 'head': '%s:%s' % (owner, new_branch), 'title': title, 'body': body, @@ -613,17 +614,9 @@ class PullRequests(models.Model): raise RuntimeError("Forwardport failure: %s (%s)" % (pr.display_name, results)) r = results - self.env.cr.commit() - new_pr = self.search([ - ('number', '=', r['number']), - ('repository.name', '=', r['base']['repo']['full_name']), - ], limit=1) - if new_pr: - _logger.info("Received forward-port PR %s", new_pr) - else: - new_pr = self._from_gh(r) - _logger.info("Created forward-port PR %s", new_pr) + new_pr = self._from_gh(r) + _logger.info("Created forward-port PR %s", new_pr) new_batch |= new_pr # delegate original author on merged original PR & on new PR so