From 2fea31883074b1c1f7c39d6119a124578ccb1ff6 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 22 Oct 2024 13:57:58 +0200 Subject: [PATCH] [IMP] runbot_merge: hide concurrent update errors As far as I can tell they are properly handled: - In `handle_status` we let the http layer retry the query, which pretty much always succeeds. - In `Commit.notify`, we rollback the application of the current commit, meaning it'll be processed by the next run of the cron, which also seems to succeed every time (that is going through the log I pretty much never notice the same commit being serialization failure'd twice in a row). Which we can trigger for faster action, this last item is not entirely necessary as statuses should generally come in fast and especially if we have concurrency errors, but it can't hurt. This means the only genuine issue is... sql_db logging a "bad query" every time there's a serialization failure. In `handle_status`, just suppress the message outright, if there's an error other than serialization the http / dispatch layer should catch and log it. In `Commit._notify` things are slightly more difficult as the execute is implicit (`flush` -> `_write` -> `execute`) so we can't pass the flag by parameter. One option would be to set and unset `_default_log_exception`, but it would either be a bit dodgy or it would require using a context manager and increasing the indentation level (or using a custom context manager). Instead just `mute_logger` the fucking thing. It's a bit brutish and mostly used in tests, but not just, and feels like the least bad option here... Closes #805 --- runbot_merge/controllers/__init__.py | 2 +- runbot_merge/models/pull_requests.py | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index eaaec321..10d815ef 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -374,7 +374,7 @@ def handle_status(env, event): SET to_check = true, statuses = c.statuses::jsonb || EXCLUDED.statuses::jsonb WHERE NOT c.statuses::jsonb @> EXCLUDED.statuses::jsonb - """, [event['sha'], status_value]) + """, [event['sha'], status_value], log_exceptions=False) env.ref("runbot_merge.process_updated_commits")._trigger() return 'ok' diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 2c97c21a..4893444c 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -23,7 +23,7 @@ from markupsafe import Markup from odoo import api, fields, models, tools, Command from odoo.exceptions import AccessError, UserError from odoo.osv import expression -from odoo.tools import html_escape, Reverse +from odoo.tools import html_escape, Reverse, mute_logger from . import commands from .utils import enum, readonly, dfm @@ -1978,9 +1978,11 @@ class Commit(models.Model): self.env.ref("runbot_merge.process_updated_commits")._trigger() return r + @mute_logger('odoo.sql_db') def _notify(self): Stagings = self.env['runbot_merge.stagings'] PRs = self.env['runbot_merge.pull_requests'] + serialization_failures = False # chances are low that we'll have more than one commit for c in self.search([('to_check', '=', True)]): sha = c.sha @@ -1999,15 +2001,18 @@ class Commit(models.Model): if stagings: stagings._notify(c) except psycopg2.errors.SerializationFailure: - _logger.info("Failed to apply commit %s (%s): serialization failure", c, sha) + serialization_failures = True + _logger.info("Failed to apply commit %s: serialization failure", sha) self.env.cr.rollback() except Exception: - _logger.exception("Failed to apply commit %s (%s)", c, sha) + _logger.exception("Failed to apply commit %s", sha) self.env.cr.rollback() else: self.env.cr.precommit.data['change-message'] = \ f"statuses changed on {sha}" self.env.cr.commit() + if serialization_failures: + self.env.ref("runbot_merge.process_updated_commits")._trigger() _sql_constraints = [ ('unique_sha', 'unique (sha)', 'no duplicated commit'),