From 3c633cb70dffd5330c5f863650ed29d1f9acc390 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 24 Oct 2018 16:14:31 +0200 Subject: [PATCH] [IMP] runbot_merge: concurrency issue with GH closing PRs being merged Once more unto the breach, with the issue of pushing stagings (with "closes" annotations) to the target branch making GH close the PR & send the hook, which makes runbot_merge consider the PR closed and the staging cancelled. This probably still doesn't fix the issue, but it reduces the problematic window: before this, the process first updates the branches, then marks the PRs, then comments & closes the PRs, and finally commits the PR update. This means as runbot_merge is sending a comment & a status update to each PR in a staging, GH has some time to send the "closed" webhook behind its back, making the controller immediately cancel the current staging, especially if the v3 endpoint is a bit slow. By moving the commenting & closing out of the critical path (to the feedback queue), this window should be significantly shortened. --- runbot_merge/models/pull_requests.py | 16 +++++++++++++--- runbot_merge/tests/test_basic.py | 1 + 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index e55b6561..ca8193b0 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -121,10 +121,14 @@ class Project(models.Model): gh = ghs[repo] = repo.github() try: - gh.comment(f.pull_request, f.message) + if f.close: + gh.close(f.pull_request, f.message) + else: + gh.comment(f.pull_request, f.message) except Exception: _logger.exception( - "Error while trying to send a comment to %s:%s (%s)", + "Error while trying to %s %s:%s (%s)", + 'close' if f.close else 'send a comment to', repo.name, f.pull_request, f.message and f.message[:200] ) @@ -836,6 +840,7 @@ class Feedback(models.Model): # objects on non-handled branches pull_request = fields.Integer() message = fields.Char() + close = fields.Boolean() class Commit(models.Model): """Represents a commit onto which statuses might be posted, @@ -1132,7 +1137,12 @@ class Stagings(models.Model): ) prs.write({'state': 'merged'}) for pr in prs: - gh[pr.repository.name].close(pr.number, 'Merged, thanks!') + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': pr.repository.id, + 'pull_request': pr.number, + 'message': "Merged, thanks!", + 'close': True, + }) finally: self.batch_ids.write({'active': False}) self.write({'active': False}) diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index ac227aaf..cf0a5af9 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -846,6 +846,7 @@ class TestMergeMethod: repo.post_status(staging.id, 'success', 'legal/cla') repo.post_status(staging.id, 'success', 'ci/runbot') env['runbot_merge.project']._check_progress() + env['runbot_merge.project']._send_feedback() assert env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), ('number', '=', prx.number)