diff --git a/libmozevent/mercurial.py b/libmozevent/mercurial.py index 84a0302..a58d879 100644 --- a/libmozevent/mercurial.py +++ b/libmozevent/mercurial.py @@ -11,7 +11,7 @@ import os import tempfile import time -from datetime import datetime, timedelta +from datetime import datetime import hglib import requests @@ -340,7 +340,6 @@ def __init__( queue_name, queue_phabricator, repositories, - diff_expiry=timedelta(hours=24), skippable_files=[], ): assert all(map(lambda r: isinstance(r, Repository), repositories.values())) @@ -348,7 +347,6 @@ def __init__( self.queue_phabricator = queue_phabricator self.repositories = repositories self.skippable_files = skippable_files - self.diff_expiry = diff_expiry def register(self, bus): self.bus = bus @@ -456,22 +454,8 @@ async def handle_build(self, repository, build): build, {"message": error_log, "duration": time.time() - start}, ) - if ( - build.diff.get("fields") - and build.diff["fields"].get("dateCreated") - and ( - datetime.now() - - datetime.fromtimestamp(build.diff["fields"]["dateCreated"]) - > self.diff_expiry - ) - ): - error_log = "This build is too old to push to try repository" - return ( - "fail:mercurial", - build, - {"message": error_log, "duration": time.time() - start}, - ) - elif build.retries: + + if build.retries: logger.warning( "Trying to apply build's diff after a remote push error " f"[{build.retries}/{MAX_PUSH_RETRIES}]" diff --git a/libmozevent/phabricator.py b/libmozevent/phabricator.py index 1767467..dd6fe0c 100644 --- a/libmozevent/phabricator.py +++ b/libmozevent/phabricator.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- import collections +from datetime import datetime, timedelta import enum import time @@ -13,6 +14,7 @@ class PhabricatorBuildState(enum.Enum): Queued = 1 Secured = 2 Public = 3 + Expired = 4 class PhabricatorBuild(object): @@ -59,7 +61,9 @@ class PhabricatorActions(object): Common Phabricator actions shared across clients """ - def __init__(self, url, api_key, retries=5, sleep=10): + def __init__( + self, url, api_key, retries=5, sleep=10, build_expiry=timedelta(hours=24) + ): self.api = PhabricatorAPI(url=url, api_key=api_key) # Phabricator secure revision retries configuration @@ -68,10 +72,12 @@ def __init__(self, url, api_key, retries=5, sleep=10): self.max_retries = retries self.retries = collections.defaultdict(lambda: (retries, None)) self.sleep = sleep + self.build_expiry = build_expiry logger.info( "Will retry Phabricator secure revision queries", retries=retries, sleep=sleep, + build_expiry=build_expiry, ) # Load secure projects @@ -114,6 +120,11 @@ def update_state(self, build): build.revision_url = self.build_revision_url(build) logger.info("Revision is public", build=str(build)) + # Check if the build has not expired + if self.is_expired_build(build): + build.state = PhabricatorBuildState.Expired + logger.info("Revision has expired", build=str(build)) + elif retries_left <= 0: # Mark as secured when no retries are left build.state = PhabricatorBuildState.Secured @@ -183,3 +194,31 @@ def build_revision_url(self, build): Build a Phabricator frontend url for a build's revision """ return "https://{}/D{}".format(self.api.hostname, build.revision_id) + + def is_expired_build(self, build): + """ + Check if a build has expired, using its Phabricator diff information + Returns True when the build has expired and should not be processed + """ + assert isinstance(build, PhabricatorBuild) + + # We need Phabricator diff details to get the date + if build.diff is None: + try: + diffs = self.api.search_diffs(diff_id=build.diff_id) + if not diffs: + raise Exception(f"Diff {build.diff_id} not found on Phabricator") + build.diff = diffs[0] + except Exception as e: + logger.warn("Failed to load diff", build=str(build), err=str(e)) + return False + + # Then we can check on the expiry date + date_created = build.diff.get("dateCreated") + if not date_created: + logger.warn("No creation date found", build=str(build)) + return False + + logger.info("Found diff creation date", build=str(build), created=date_created) + + return datetime.now() - datetime.fromtimestamp(date_created) > self.build_expiry diff --git a/tests/conftest.py b/tests/conftest.py index 6b0916d..a94a97c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -129,8 +129,12 @@ def _diff_search(request): if "revisionPHIDs" in params["constraints"]: # Search from revision mock_name = "search-{}".format(params["constraints"]["revisionPHIDs"][0]) + elif "ids" in params["constraints"]: + # Search from diff IDs + diffs = "-".join(map(str, params["constraints"]["ids"])) + mock_name = "search-{}".format(diffs) elif "phids" in params["constraints"]: - # Search from diffs + # Search from diff PHIDs diffs = "-".join(params["constraints"]["phids"]) mock_name = "search-{}".format(diffs) else: diff --git a/tests/mocks/phabricator/search-1234.json b/tests/mocks/phabricator/search-1234.json new file mode 100644 index 0000000..c0d49fa --- /dev/null +++ b/tests/mocks/phabricator/search-1234.json @@ -0,0 +1,35 @@ +{ + "result": { + "data": [ + { + "id": 1234, + "type": "DIFF", + "phid": "PHID-DIFF-c0ffee", + "fields": { + "revisionPHID": "PHID-DREV-xxx", + "authorPHID": "PHID-USER-yyy", + "repositoryPHID": "PHID-REPO-zzz", + "refs": [], + "dateCreated": 1510251135, + "dateModified": 1510251135, + "policy": { + "view": "public" + } + }, + "attachments": {} + } + ], + "maps": {}, + "query": { + "queryKey": null + }, + "cursor": { + "limit": 100, + "after": null, + "before": null, + "order": null + } + }, + "error_code": null, + "error_info": null +} diff --git a/tests/test_mercurial.py b/tests/test_mercurial.py index 25a48e3..dd74dfd 100644 --- a/tests/test_mercurial.py +++ b/tests/test_mercurial.py @@ -917,64 +917,3 @@ def test_get_base_identifier(mock_mc): assert ( mock_mc.get_base_identifier(stack) == "tip" ), "`tip` commit should be used when `use_latest_revision` is `True`." - - -@responses.activate -@pytest.mark.asyncio -async def test_push_failure_diff_expiry(PhabricatorMock, mock_mc): - diff = { - "revisionPHID": "PHID-DREV-badutf8", - "baseRevision": "missing", - "phid": "PHID-DIFF-badutf8", - "id": 555, - # a date in 2017 - "fields": {"dateCreated": 1510251135}, - } - build = MockBuild(4444, "PHID-REPO-mc", 5555, "PHID-build-badutf8", diff) - with PhabricatorMock as phab: - phab.load_patches_stack(build) - - bus = MessageBus() - bus.add_queue("phabricator") - - from libmozevent import mercurial - - mercurial.TRY_STATUS_URL = "http://test.status/try" - - sleep_history = [] - - class AsyncioMock(object): - async def sleep(self, value): - nonlocal sleep_history - sleep_history.append(value) - - mercurial.asyncio = AsyncioMock() - - responses.get( - "http://test.status/try", status=200, json={"result": {"status": "open"}} - ) - - repository_mock = MagicMock(spec=Repository) - - worker = MercurialWorker( - "mercurial", "phabricator", repositories={"PHID-REPO-mc": repository_mock} - ) - worker.register(bus) - - await bus.send("mercurial", build) - assert bus.queues["mercurial"].qsize() == 1 - task = asyncio.create_task(worker.run()) - - # Check the treeherder link was queued - mode, out_build, details = await bus.receive("phabricator") - task.cancel() - - assert build.retries == 0 - - assert mode == "fail:mercurial" - assert out_build == build - assert details["duration"] > 0 - assert details["message"] == "This build is too old to push to try repository" - - # no call sent to TRY_STATUS_URL - assert len(responses.calls) == 0 diff --git a/tests/test_phabricator.py b/tests/test_phabricator.py index 2705b5b..bca96e7 100644 --- a/tests/test_phabricator.py +++ b/tests/test_phabricator.py @@ -49,3 +49,15 @@ def test_backoff(PhabricatorMock): phab.update_state(build) assert build.state == PhabricatorBuildState.Public assert phab.is_visible.call_count == 3 + + +def test_expiry(PhabricatorMock, caplog): + """ + Test diff expiration method on a known API mock + """ + build = MockBuild(1234, "PHID-REPO-mc", 5678, "PHID-HMBT-deadbeef", {}) + build.state = PhabricatorBuildState.Queued + build.diff = None # Force loading the mockup + + with PhabricatorMock as phab: + assert phab.is_expired_build(build)