diff --git a/bot/code_review_bot/analysis.py b/bot/code_review_bot/analysis.py index f6a9a6a7c..e6b33427d 100644 --- a/bot/code_review_bot/analysis.py +++ b/bot/code_review_bot/analysis.py @@ -1,7 +1,7 @@ from functools import cached_property import structlog -from libmozdata.phabricator import BuildState, UnitResult, UnitResultState +from libmozdata.phabricator import BuildState, ConduitError, UnitResult, UnitResultState from code_review_bot.sources.phabricator import PhabricatorBuild, PhabricatorBuildState @@ -146,12 +146,21 @@ def publish_analysis_phabricator(payload, phabricator_api): "Missing base revision on PhabricatorBuild, adding a warning to Unit Tests section on Phabricator" ) - phabricator_api.create_harbormaster_uri( - build.target_phid, - "treeherder", - "CI (Treeherder) Jobs", - extras["treeherder_url"], - ) + try: + phabricator_api.create_harbormaster_uri( + build.target_phid, + "treeherder", + "CI (Treeherder) Jobs", + extras["treeherder_url"], + ) + except ConduitError as e: + if e.error_info and "Duplicate entry" in e.error_info: + logger.warning( + "Harbormaster URI artifact already exists, skipping creation (retry?)", + build=str(build), + ) + else: + raise elif mode == "work": phabricator_api.update_build_target(build.target_phid, BuildState.Work) diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index 3face72be..465ca3fd7 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -7,7 +7,7 @@ from itertools import groupby import structlog -from libmozdata.phabricator import BuildState, PhabricatorAPI +from libmozdata.phabricator import BuildState, ConduitError, PhabricatorAPI from taskcluster.utils import stringDate from code_review_bot import Level, stats @@ -760,6 +760,16 @@ def publish_link(self, revision: Revision, slug: str, name: str, url: str): ) return - self.phabricator.create_harbormaster_uri( - revision.build_target_phid, slug, name, url - ) + try: + self.phabricator.create_harbormaster_uri( + revision.build_target_phid, slug, name, url + ) + except ConduitError as e: + if e.error_info and "Duplicate entry" in e.error_info: + logger.warning( + "Harbormaster URI artifact already exists, skipping creation (retry?)", + slug=slug, + url=url, + ) + else: + raise diff --git a/bot/tests/test_phabricator_analysis.py b/bot/tests/test_phabricator_analysis.py index 9eb499df8..5e64eec6c 100644 --- a/bot/tests/test_phabricator_analysis.py +++ b/bot/tests/test_phabricator_analysis.py @@ -4,8 +4,15 @@ import json import tempfile +from unittest import mock + +import pytest +from libmozdata.phabricator import ConduitError from code_review_bot import mercurial +from code_review_bot.analysis import ( + publish_analysis_phabricator, +) from code_review_bot.config import RepositoryConf from code_review_bot.revisions import PhabricatorRevision from code_review_bot.sources.phabricator import PhabricatorActions @@ -220,3 +227,46 @@ def mock_hgrun(cmd): # Reset settings for following tests mock_config.mercurial_cache = None mock_config.ssh_key = None + + +def test_publish_analysis_phabricator_duplicate_harbormaster_uri(): + """ + When create_harbormaster_uri raises a ConduitError due to a duplicate key + (e.g. on task retry after worker shutdown), the error should be swallowed + and a warning logged instead of crashing the publication task. + """ + build = mock.MagicMock() + build.target_phid = "PHID-HMBT-test" + build.missing_base_revision = False + + phabricator_api = mock.MagicMock() + phabricator_api.create_harbormaster_uri.side_effect = ConduitError( + "Duplicate entry", + error_code="ERR-CONDUIT-CORE", + error_info="Duplicate entry 'uri-VEVIsJOfD0wc' for key 'harbormaster_buildartifact.key_artifact'", + ) + + payload = ("success", build, {"treeherder_url": "https://treeherder.mozilla.org/"}) + publish_analysis_phabricator(payload, phabricator_api) + + phabricator_api.create_harbormaster_uri.assert_called_once() + + +def test_publish_analysis_phabricator_reraises_other_conduit_errors(): + """ + Non-duplicate ConduitErrors must still propagate. + """ + build = mock.MagicMock() + build.target_phid = "PHID-HMBT-test" + build.missing_base_revision = False + + phabricator_api = mock.MagicMock() + phabricator_api.create_harbormaster_uri.side_effect = ConduitError( + "Some other error", + error_code="ERR-CONDUIT-CORE", + error_info="Some unrelated error", + ) + + payload = ("success", build, {"treeherder_url": "https://treeherder.mozilla.org/"}) + with pytest.raises(ConduitError): + publish_analysis_phabricator(payload, phabricator_api) diff --git a/bot/tests/test_workflow.py b/bot/tests/test_workflow.py index 1cd07713d..38bb1414f 100644 --- a/bot/tests/test_workflow.py +++ b/bot/tests/test_workflow.py @@ -8,8 +8,10 @@ import pytest import responses +from libmozdata.phabricator import ConduitError from code_review_bot.config import Settings +from code_review_bot.revisions import PhabricatorRevision from code_review_bot.tasks.clang_format import ClangFormatIssue, ClangFormatTask from code_review_bot.tasks.clang_tidy import ClangTidyTask from code_review_bot.tasks.clang_tidy_external import ExternalTidyTask @@ -168,3 +170,48 @@ def test_before_after(mock_taskcluster_config, mock_workflow, mock_task, mock_re ] assert issues[0].new_issue is True assert issues[1].new_issue is False + + +def test_publish_link_duplicate_harbormaster_uri(mock_workflow): + """ + When publish_link raises a ConduitError due to a duplicate Harbormaster URI + artifact (e.g. on task retry after worker shutdown), the error should be + swallowed and a warning logged. + """ + mock_workflow.update_build = True + mock_workflow.phabricator = mock.MagicMock() + mock_workflow.phabricator.create_harbormaster_uri.side_effect = ConduitError( + "Duplicate entry", + error_code="ERR-CONDUIT-CORE", + error_info="Duplicate entry 'uri-VEVIsJOfD0wc' for key 'harbormaster_buildartifact.key_artifact'", + ) + + revision = mock.MagicMock(spec=PhabricatorRevision) + revision.build_target_phid = "PHID-HMBT-test" + + mock_workflow.publish_link( + revision, "treeherder", "CI Jobs", "https://treeherder.mozilla.org/" + ) + + mock_workflow.phabricator.create_harbormaster_uri.assert_called_once() + + +def test_publish_link_reraises_other_conduit_errors(mock_workflow): + """ + Non-duplicate ConduitErrors must still propagate from publish_link. + """ + mock_workflow.update_build = True + mock_workflow.phabricator = mock.MagicMock() + mock_workflow.phabricator.create_harbormaster_uri.side_effect = ConduitError( + "Some other error", + error_code="ERR-CONDUIT-CORE", + error_info="Some unrelated error", + ) + + revision = mock.MagicMock(spec=PhabricatorRevision) + revision.build_target_phid = "PHID-HMBT-test" + + with pytest.raises(ConduitError): + mock_workflow.publish_link( + revision, "treeherder", "CI Jobs", "https://treeherder.mozilla.org/" + )