Check build expiry in Phabricator state update (#109)

Co-authored-by: Bastien Abadie <[email protected]>

Author: La0

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,15 +340,13 @@ def __init__(
         queue_name,
         queue_phabricator,
         repositories,
-        diff_expiry=timedelta(hours=24),
         skippable_files=[],
     ):
         assert all(map(lambda r: isinstance(r, Repository), repositories.values()))
         self.queue_name = queue_name
         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}]"

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

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:

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
+}

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

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)