From 668ff6115f84ded182bde1e0e4c141f590b66397 Mon Sep 17 00:00:00 2001 From: Bastien Abadie Date: Tue, 26 Nov 2024 13:28:46 +0100 Subject: [PATCH 1/5] Check build expiry in Phabricator state update --- libmozevent/mercurial.py | 19 +----------- libmozevent/phabricator.py | 39 +++++++++++++++++++++++- tests/test_mercurial.py | 61 -------------------------------------- 3 files changed, 39 insertions(+), 80 deletions(-) diff --git a/libmozevent/mercurial.py b/libmozevent/mercurial.py index 84a0302..c7b3ebb 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,21 +454,6 @@ 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: logger.warning( "Trying to apply build's diff after a remote push error " diff --git a/libmozevent/phabricator.py b/libmozevent/phabricator.py index 1767467..d9400c3 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 not self.check_build_expiry(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,29 @@ 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 check_build_expiry(self, build): + """ + Check if a build has expired, using its Phabricator diff information + Returns False 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=self.diff_id) + build.diff = diffs[0] + except Exception as e: + logger.warn("Failed to load diff", build=str(build), err=str(e)) + return True + + # Then we can check on the expiry date + date_created = build.diff.get("fields", {}).get("dateCreated") + if not date_created: + logger.warn("No creation date found", build=str(build)) + return True + + return ( + datetime.now() - datetime.fromtimestamp(date_created) <= self.build_expiry + ) 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 From 193492e555b5ac47c08ca9a2a30f4b0ded191394 Mon Sep 17 00:00:00 2001 From: Bastien Abadie Date: Thu, 28 Nov 2024 14:46:02 +0100 Subject: [PATCH 2/5] Add unit test and fix bugs --- libmozevent/phabricator.py | 6 ++-- tests/conftest.py | 6 +++- tests/mocks/phabricator/search-1234.json | 35 ++++++++++++++++++++++++ tests/test_phabricator.py | 12 ++++++++ 4 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 tests/mocks/phabricator/search-1234.json diff --git a/libmozevent/phabricator.py b/libmozevent/phabricator.py index d9400c3..0f7c2df 100644 --- a/libmozevent/phabricator.py +++ b/libmozevent/phabricator.py @@ -205,18 +205,20 @@ def check_build_expiry(self, build): # We need Phabricator diff details to get the date if build.diff is None: try: - diffs = self.api.search_diffs(diff_id=self.diff_id) + diffs = self.api.search_diffs(diff_id=build.diff_id) build.diff = diffs[0] except Exception as e: logger.warn("Failed to load diff", build=str(build), err=str(e)) return True # Then we can check on the expiry date - date_created = build.diff.get("fields", {}).get("dateCreated") + date_created = build.diff.get("dateCreated") if not date_created: logger.warn("No creation date found", build=str(build)) return True + 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_phabricator.py b/tests/test_phabricator.py index 2705b5b..0017f57 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 not phab.check_build_expiry(build) From 6179effb24672b82a7a83cb8f8c016cd2d926853 Mon Sep 17 00:00:00 2001 From: Bastien Abadie Date: Thu, 28 Nov 2024 16:45:50 +0100 Subject: [PATCH 3/5] Explicit error message --- libmozevent/phabricator.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libmozevent/phabricator.py b/libmozevent/phabricator.py index 0f7c2df..b5c0807 100644 --- a/libmozevent/phabricator.py +++ b/libmozevent/phabricator.py @@ -206,6 +206,8 @@ def check_build_expiry(self, build): 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)) From 17c7a1779791df71d2028c9668806bc1c39e4646 Mon Sep 17 00:00:00 2001 From: Bastien Abadie Date: Wed, 15 Jan 2025 15:21:26 +0100 Subject: [PATCH 4/5] Suggestion on if/elif --- libmozevent/mercurial.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libmozevent/mercurial.py b/libmozevent/mercurial.py index c7b3ebb..a58d879 100644 --- a/libmozevent/mercurial.py +++ b/libmozevent/mercurial.py @@ -454,7 +454,8 @@ async def handle_build(self, repository, build): 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}]" From 5b5a18b573f7137c1202582747d9a61471e67339 Mon Sep 17 00:00:00 2001 From: Bastien Abadie Date: Wed, 15 Jan 2025 15:26:45 +0100 Subject: [PATCH 5/5] Rename check_build_expiry to is_build_expired --- libmozevent/phabricator.py | 14 ++++++-------- tests/test_phabricator.py | 2 +- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/libmozevent/phabricator.py b/libmozevent/phabricator.py index b5c0807..dd6fe0c 100644 --- a/libmozevent/phabricator.py +++ b/libmozevent/phabricator.py @@ -121,7 +121,7 @@ def update_state(self, build): logger.info("Revision is public", build=str(build)) # Check if the build has not expired - if not self.check_build_expiry(build): + if self.is_expired_build(build): build.state = PhabricatorBuildState.Expired logger.info("Revision has expired", build=str(build)) @@ -195,10 +195,10 @@ def build_revision_url(self, build): """ return "https://{}/D{}".format(self.api.hostname, build.revision_id) - def check_build_expiry(self, build): + def is_expired_build(self, build): """ Check if a build has expired, using its Phabricator diff information - Returns False when the build has expired and should not be processed + Returns True when the build has expired and should not be processed """ assert isinstance(build, PhabricatorBuild) @@ -211,16 +211,14 @@ def check_build_expiry(self, build): build.diff = diffs[0] except Exception as e: logger.warn("Failed to load diff", build=str(build), err=str(e)) - return True + 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 True + return False logger.info("Found diff creation date", build=str(build), created=date_created) - return ( - datetime.now() - datetime.fromtimestamp(date_created) <= self.build_expiry - ) + return datetime.now() - datetime.fromtimestamp(date_created) > self.build_expiry diff --git a/tests/test_phabricator.py b/tests/test_phabricator.py index 0017f57..bca96e7 100644 --- a/tests/test_phabricator.py +++ b/tests/test_phabricator.py @@ -60,4 +60,4 @@ def test_expiry(PhabricatorMock, caplog): build.diff = None # Force loading the mockup with PhabricatorMock as phab: - assert not phab.check_build_expiry(build) + assert phab.is_expired_build(build)