2026 11 09 tjr bugzilla retry commandeered

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:

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))

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))
 

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: <differential.revision.edit> 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):