From 51ded6e6f04977dd1d8782d7fcbb8e82598ea708 Mon Sep 17 00:00:00 2001 From: Tom Ritter Date: Mon, 9 Nov 2026 18:57:31 +0000 Subject: [PATCH 1/2] Leave a Phabricator revision alone if someone has commandeered it. Fixes #397 Phabricator won't let us abandon a revision we're no longer the author of. That error was retried and then reported on the bug as a failure to abandon it; now we log it and move on, since the revision is the new author's to keep or abandon. --- apis/phabricator.py | 3 ++ tests/functionality_all_platforms.py | 51 ++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/apis/phabricator.py b/apis/phabricator.py index 93e1e50b..77cf9940 100644 --- a/apis/phabricator.py +++ b/apis/phabricator.py @@ -207,5 +207,8 @@ def abandon(self, phab_revision): if result['error']: if "You can not abandon this revision because it has already been closed." in result['errorMessage']: self.logger.log("Strangely, the phabricator revision %s was already closed when we tried to abandon it. Oh well." % phab_revision, level=LogLevel.Warning) + elif "You can not abandon this revision because you are not the author." in result['errorMessage']: + # Someone commandeered the revision, so it's theirs to keep or abandon + self.logger.log("The phabricator revision %s has been commandeered by someone else, so we're leaving it alone instead of abandoning it." % phab_revision, level=LogLevel.Info) else: raise Exception("Got an error from phabricator when trying to abandon %s: %s" % (phab_revision, result)) diff --git a/tests/functionality_all_platforms.py b/tests/functionality_all_platforms.py index f1339578..fc97f47a 100755 --- a/tests/functionality_all_platforms.py +++ b/tests/functionality_all_platforms.py @@ -5,6 +5,7 @@ # file, You can obtain one at http://mozilla.org/MPL/2.0/. import sys +import json import copy import inspect import unittest @@ -1271,6 +1272,56 @@ def abandon_callback(cmd): finally: self._cleanup(u, expected_values) + # Create -> Build Failed, but someone commandeered the Phabricator revision (Issue #397) + @logEntryExitHeaderLine + def testExistingJobBuildFailedRevisionCommandeered(self): + @treeherder_response + def treeherder(request_type, fullpath): + if request_type == TYPE_HEALTH: + return "health_build_failed.txt" + elif request_type == TYPE_BUGSUGGESTIONS: + return "bugsuggestions_build_failure.txt" + else: # TYPE_JOBS + return "jobs_build_failed.txt" + + abandon_calls = [0] + + def abandon_callback(cmd): + abandon_calls[0] += 1 + # What Phabricator answered in https://github.com/mozilla-services/updatebot/issues/397 + return json.dumps({ + "error": "ERR-CONDUIT-CORE", + "errorMessage": "ERR-CONDUIT-CORE: Validation errors:\n - You can not abandon this revision because you are not the author. You can only abandon revisions you own. You can change this behavior by adjusting the \"differential.always-allow-abandon\" setting in Config.", + "response": None}) + + library_filter = 'dav1d' + (u, expected_values, _check_jobs) = self._setup( + library_filter, + lambda b: ["55ca6286e3e4f4fba5d0448333fa99fc5a404a73|2021-02-09 15:30:04 -0500|2021-02-12 17:40:01 +0000"], + lambda: 50, # get_filed_bug_id_func, + lambda b: {}, # filed_bug_ids_func + treeherder, + command_callbacks={'abandon': abandon_callback} + ) + comments = [] + original_comment_on_bug = u.bugzillaProvider.comment_on_bug + u.bugzillaProvider.comment_on_bug = lambda bug_id, comment, *args, **kwargs: (comments.append(comment), original_comment_on_bug(bug_id, comment, *args, **kwargs)) + exceptions = [] + original_log_exception = u.logger.log_exception + u.logger.log_exception = lambda e: (exceptions.append(e), original_log_exception(e)) + + try: + u.run(library_filter=library_filter) + _check_jobs(JOBSTATUS.AWAITING_SECOND_PLATFORMS_TRY_RESULTS, JOBOUTCOME.PENDING) + + u.run(library_filter=library_filter) + _check_jobs(JOBSTATUS.DONE, JOBOUTCOME.BUILD_FAILED) + self.assertEqual(abandon_calls[0], 1, "Shouldn't retry abandoning a revision someone else owns") + self.assertEqual([c for c in comments if "error while trying to abandon" in c], [], "Shouldn't report failing to abandon a commandeered revision") + self.assertEqual(exceptions, []) + finally: + self._cleanup(u, expected_values) + # Create -> Build Failed because of an intermittent -> Retrigger the build (Issue #453) # The jobs list, push health and bug suggestions are the real Treeherder responses for # the try push of https://bugzilla.mozilla.org/show_bug.cgi?id=2089270 (harfbuzz 15.1.0): From 3d1e43b6afb8648bbcec465ea9a17f740bd4ef14 Mon Sep 17 00:00:00 2001 From: Tom Ritter Date: Mon, 9 Nov 2026 18:57:31 +0000 Subject: [PATCH 2/2] Retry Bugzilla writes that fail with a gateway error, without repeating them. Fixes #401 Bugzilla's front end sometimes fails a request with a 502, 503 or 504 (e.g. '504 stream timeout'). Retry filing bugs, commenting and closing bugs when that happens, but first check whether the failed attempt went through anyway, so we don't file or comment twice: a run's bugs and comments are marked with its task id. (The response's status code is already in the error, since _load_json_or_raise.) --- apis/bugzilla_api.py | 75 +++++++++++++++++++++++++++++++-- tests/bugzilla.py | 99 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 170 insertions(+), 4 deletions(-) diff --git a/apis/bugzilla_api.py b/apis/bugzilla_api.py index 46790ad8..c7bde18a 100644 --- a/apis/bugzilla_api.py +++ b/apis/bugzilla_api.py @@ -30,6 +30,55 @@ def task_log_url(): return root.rstrip('/') + '/tasks/' + os.environ['TASK_ID'] +class BugzillaGatewayError(Exception): + """Bugzilla's front end failed the request (e.g. '504 stream timeout'). The request + may or may not have been processed.""" + pass + + +# Bugzilla's front end answers with these when a request times out or doesn't reach +# Bugzilla. The ones we've seen weren't processed, but that can't be relied on, so +# before retrying a write we check whether the failed attempt went through. +GATEWAY_STATUSES = (502, 503, 504) + + +def _raise_if_gateway_error(r, where): + if getattr(r, "status_code", None) not in GATEWAY_STATUSES: + return + message = "%s: Bugzilla's front end failed the request (status=%s): %s" % (where, r.status_code, r.text[:2000]) + # We can only tell whether a write went through from the task id it was marked + # with, so outside of Taskcluster we don't retry it + if "TASK_ID" not in os.environ: + raise Exception(message) + raise BugzillaGatewayError(message) + + +def _bug_filed_by_this_task(url, apikey, summary): + r = requests.get(url + "bug", params={ + 'api_key': apikey, + 'f1': 'status_whiteboard', 'o1': 'substring', 'v1': 'task_id: ' + os.environ['TASK_ID'], + 'f2': 'short_desc', 'o2': 'equals', 'v2': summary, + 'include_fields': 'id'}) + # A gateway error here is retried too, and the next attempt checks again + _raise_if_gateway_error(r, "fileBug check") + bugs = _load_json_or_raise(r, "fileBug check")['bugs'] + return bugs[0]['id'] if bugs else None + + +def _comment_posted_by_this_task(url, apikey, bugID, comment): + r = requests.get(url + "bug/" + str(bugID) + "/comment", params={'api_key': apikey}) + _raise_if_gateway_error(r, "comment check") + bugs = _load_json_or_raise(r, "comment check")['bugs'] + for c in bugs.get(str(bugID), {}).get('comments', []): + if os.environ['TASK_ID'] in c.get('tags', []) and c.get('text', '').strip() == comment.strip(): + return True + return False + + +# Writes are retried only for gateway errors, with checks that they didn't go through +retry_write = retry(times=3, sleep_s=30, exceptions=(BugzillaGatewayError,)) + + class CommentRejectedError(Exception): """Bugzilla's web filter (not Bugzilla itself) refused the comment, with an empty 406 response. It has done this for comments quoting shell trace output.""" @@ -62,9 +111,15 @@ def _load_json_or_raise(r, where: str): ) from e -def fileBug(url, apikey, ff_version, product, component, summary, description, cc_list, needinfo, see_also, depends_on, blocks, moco_confidential, mark_affected=True): +@retry_write +def fileBug(url, apikey, ff_version, product, component, summary, description, cc_list, needinfo, see_also, depends_on, blocks, moco_confidential, mark_affected=True, retry_attempt=1): assert isinstance(cc_list, list) + if retry_attempt > 1: + filed_id = _bug_filed_by_this_task(url, apikey, summary) + if filed_id: + return filed_id + data = { 'version': "unspecified", 'op_sys': "unspecified", @@ -102,6 +157,7 @@ def fileBug(url, apikey, ff_version, product, component, summary, description, c }) r = requests.post(url + "bug?api_key=" + apikey, json=data) + _raise_if_gateway_error(r, "fileBug") j = _load_json_or_raise(r, "fileBug") @@ -111,7 +167,11 @@ def fileBug(url, apikey, ff_version, product, component, summary, description, c raise Exception(j) -def commentOnBug(url, apikey, bugID, comment, needinfo=None, assignee=None): +@retry_write +def commentOnBug(url, apikey, bugID, comment, needinfo=None, assignee=None, retry_attempt=1): + if retry_attempt > 1 and _comment_posted_by_this_task(url, apikey, bugID, comment): + return + data = { 'id': bugID, 'comment': {'body': comment}, @@ -132,6 +192,7 @@ def commentOnBug(url, apikey, bugID, comment, needinfo=None, assignee=None): json=data ) + _raise_if_gateway_error(r, "commentOnBug") if r.status_code == 406: raise CommentRejectedError("Bugzilla rejected the comment on Bug %s (status=406)" % bugID) @@ -145,8 +206,12 @@ def commentOnBug(url, apikey, bugID, comment, needinfo=None, assignee=None): raise Exception(j) -def closeBug(url, apikey, bugID, resolution, comment, dup_id=None): +@retry_write +def closeBug(url, apikey, bugID, resolution, comment, dup_id=None, retry_attempt=1): assert dup_id is None or resolution == 'DUPLICATE' + # Closing the bug and its comment happen together, so finding the comment means it closed + if retry_attempt > 1 and _comment_posted_by_this_task(url, apikey, bugID, comment): + return data = { 'id': bugID, 'status': 'RESOLVED', @@ -163,6 +228,7 @@ def closeBug(url, apikey, bugID, resolution, comment, dup_id=None): json=data ) + _raise_if_gateway_error(r, "closeBug") j = _load_json_or_raise(r, "closeBug") if 'bugs' in j: @@ -200,6 +266,7 @@ def openBugsMetadata(url, bugIDs): raise Exception(j) from e +@retry_write def markFFVersionAffected(url, apikey, bugID, ff_version, affected): if not is_prod(url): return @@ -214,6 +281,8 @@ def markFFVersionAffected(url, apikey, bugID, ff_version, affected): json=data ) + # Setting a field again is harmless, so a retry needs no check + _raise_if_gateway_error(r, "markFFVersionAffected") j = _load_json_or_raise(r, "markFFVersionAffected") if 'bugs' in j: diff --git a/tests/bugzilla.py b/tests/bugzilla.py index ea77ede7..051abcf2 100755 --- a/tests/bugzilla.py +++ b/tests/bugzilla.py @@ -8,6 +8,7 @@ import re import sys import unittest +import urllib.parse from threading import Thread from http import server @@ -19,6 +20,7 @@ from components.bugzilla import BugzillaProvider, CommentTemplates from components.logging import SimpleLoggerConfig +import components.utilities from apis.bugzilla_api import task_id_whiteboard, CommentRejectedError TRY_REVISION = "this-is-my-try-link" @@ -28,12 +30,37 @@ class MockBugzillaServer(server.BaseHTTPRequestHandler): closed_bug_ids = [] filed_bugs = [] received_comments = {} + # Writes that failed with a gateway error, but that Bugzilla processed anyway + processed_bugs = {} + processed_comments = {} + comment_gets = {} + + def _gateway_error(self, status): + self.send_response(status) + self.send_header("Content-type", "text/plain") + self.end_headers() + self.wfile.write(b"stream timeout") def do_POST(self): expectedPath_file = "/bug?api_key=bob" size = int(self.headers.get('content-length')) content = json.loads(self.rfile.read(size).decode("utf-8")) + # Filing fails with a 502 the first time; 'gateway-processed' was filed anyway + if 'gateway-' in content.get('summary', ''): + MockBugzillaServer.filed_bugs.append(content) + attempts = len([b for b in MockBugzillaServer.filed_bugs if b['summary'] == content['summary']]) + if attempts == 1: + if 'gateway-processed' in content['summary']: + MockBugzillaServer.processed_bugs[content['summary']] = 460 + self._gateway_error(502) + return + self.send_response(200) + self.send_header("Content-type", "application/json") + self.end_headers() + self.wfile.write(b'{"id":459}') + return + self.send_response(200) self.send_header("Content-type", "application/json") self.end_headers() @@ -83,11 +110,26 @@ def do_GET(self): expectedPath_comments_github = "/bug/7893/comment" expectedPath_comments_mozilla_central = "/bug/7894/comment" + # Checking bug 1233's comments fails with a gateway error the first time + if self.path.startswith("/bug/1233/comment"): + MockBugzillaServer.comment_gets["1233"] = MockBugzillaServer.comment_gets.get("1233", 0) + 1 + if MockBugzillaServer.comment_gets["1233"] == 1: + self._gateway_error(504) + return + self.send_response(200) self.send_header("Content-type", "application/json") self.end_headers() - if expectedPath_find in self.path: + query = urllib.parse.parse_qs(urllib.parse.urlparse(self.path).query) + if query.get('f1') == ['status_whiteboard']: + # Looking for a bug filed by this task with this summary + bug_id = MockBugzillaServer.processed_bugs.get(query['v2'][0]) + self.wfile.write(json.dumps({'bugs': [{'id': bug_id}] if bug_id else []}).encode()) + elif re.match(r"/bug/(1231|1233|7895)/comment", self.path): + bug_id = re.match(r"/bug/([0-9]+)/", self.path).group(1) + self.wfile.write(json.dumps({'bugs': {bug_id: {'comments': MockBugzillaServer.processed_comments.get(bug_id, [])}}}).encode()) + elif expectedPath_find in self.path: self.wfile.write('{"bugs":[{"id":2,"assigned_to_detail":{"id":578488,"email":"tom@mozilla.com","real_name":"Tom Ritter [:tjr]","name":"tom@mozilla.com","nick":"tjr"},"assigned_to":"tom@mozilla.com"}],"faults":[]}'.encode()) elif expectedPath_comments_autoland == self.path: self.wfile.write('{"bugs":{"7892":{"comments":[{"text":"landing: https://hg.mozilla.org/integration/autoland/rev/abc123"}]}}}'.encode()) @@ -109,6 +151,16 @@ def do_PUT(self): bug_id = re.match(r"/bug/([0-9]+)\?api_key=bob", self.path).groups(0)[0] MockBugzillaServer.received_comments.setdefault(bug_id, []).append(content) + # Gateway errors: bugs 1230 and 1233 fail once, bug 1231 fails but is processed + # anyway, bug 1232 always fails, and closing bug 7895 fails but is processed anyway + attempts = len(MockBugzillaServer.received_comments[bug_id]) + if bug_id in ("1231", "7895"): + MockBugzillaServer.processed_comments.setdefault(bug_id, []).append( + {'text': content['comment']['body'], 'tags': content['comment_tags']}) + if (bug_id in ("1230", "1233") and attempts == 1) or bug_id in ("1231", "1232", "7895"): + self._gateway_error(504) + return + # Like Bugzilla's web filter: an empty 406. Bug 1238 rejects everything # but Updatebot's short replacement comment; bug 1239 rejects everything. if bug_id == "1239" or (bug_id == "1238" and "Bugzilla rejected it" not in content['comment']['body']): @@ -274,6 +326,51 @@ def testVendorCommandsIncludeTheRevision(self): ]: self.assertIn("`%s`" % command, comment) + def _with_task_id(self, func): + os.environ['TASK_ID'] = 'abc123' + original_sleep = components.utilities.time.sleep + components.utilities.time.sleep = lambda s: None + try: + return func() + finally: + components.utilities.time.sleep = original_sleep + del os.environ['TASK_ID'] + + def testCommentRetriedAfterGatewayError(self): + self._with_task_id(lambda: self.bugzillaProvider.comment_on_bug(1230, "Retried comment")) + self.assertEqual(len(MockBugzillaServer.received_comments["1230"]), 2) + + def testCommentNotRepeatedIfProcessed(self): + # The comment failed with a 504 but Bugzilla posted it, so posting it again would duplicate it + self._with_task_id(lambda: self.bugzillaProvider.comment_on_bug(1231, "Processed comment")) + self.assertEqual(len(MockBugzillaServer.received_comments["1231"]), 1) + + def testCommentGatewayErrorGivesUp(self): + with self.assertRaisesRegex(Exception, "status=504"): + self._with_task_id(lambda: self.bugzillaProvider.comment_on_bug(1232, "Failing comment")) + self.assertEqual(len(MockBugzillaServer.received_comments["1232"]), 3) + + def testFileRetriedAfterGatewayError(self): + library = Struct(name='gw', bugzilla_product='Core', bugzilla_component='ImageLib') + self.assertEqual(self._with_task_id(lambda: self.bugzillaProvider.file_bug(library, "Update gw to gateway-retry", "", [])), 459) + self.assertEqual(len([b for b in MockBugzillaServer.filed_bugs if b['summary'] == "Update gw to gateway-retry"]), 2) + + def testFileNotRepeatedIfProcessed(self): + library = Struct(name='gw', bugzilla_product='Core', bugzilla_component='ImageLib') + self.assertEqual(self._with_task_id(lambda: self.bugzillaProvider.file_bug(library, "Update gw to gateway-processed", "", [])), 460) + self.assertEqual(len([b for b in MockBugzillaServer.filed_bugs if b['summary'] == "Update gw to gateway-processed"]), 1) + + def testGatewayErrorWhileCheckingIsRetried(self): + # The comment fails, then so does checking whether it was posted; the last + # attempt checks again, finds it wasn't, and posts it + self._with_task_id(lambda: self.bugzillaProvider.comment_on_bug(1233, "Retried comment")) + self.assertEqual(MockBugzillaServer.comment_gets["1233"], 2) + self.assertEqual(len(MockBugzillaServer.received_comments["1233"]), 2) + + def testCloseNotRepeatedIfProcessed(self): + self._with_task_id(lambda: self.bugzillaProvider.wontfix_bug(7895, "Closing")) + self.assertEqual(len(MockBugzillaServer.received_comments["7895"]), 1) + def testCommentRejectedWithoutTaskId(self): self.assertIn("log of the Updatebot run", CommentTemplates.COMMENT_REJECTED(None))