launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/webhook-retries-manual into lp:launchpad with lp:~wgrant/launchpad/webhook-retries as a prerequisite.

Commit message:
Add and export WebhookDeliveryJob.retry() to immediate force another attempt.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #342729 in Launchpad itself: "Should support post-commit webhooks"
  https://bugs.launchpad.net/launchpad/+bug/342729

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/webhook-retries-manual/+merge/266195

Add and export WebhookDeliveryJob.retry() to immediate force another attempt.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/webhook-retries-manual into lp:launchpad.

=== modified file 'lib/lp/services/job/model/job.py'
--- lib/lp/services/job/model/job.py	2015-07-29 09:06:46 +0000
+++ lib/lp/services/job/model/job.py	2015-07-29 09:06:47 +0000
@@ -203,7 +203,8 @@
                 transaction.abort()
             # Commit the transaction to update the DB time.
             transaction.commit()
-        self._set_status(JobStatus.WAITING)
+        if self.status != JobStatus.WAITING:
+            self._set_status(JobStatus.WAITING)
         self.date_finished = datetime.datetime.now(UTC)
         if add_commit_hook is not None:
             add_commit_hook()

=== modified file 'lib/lp/services/webhooks/interfaces.py'
--- lib/lp/services/webhooks/interfaces.py	2015-07-29 09:06:46 +0000
+++ lib/lp/services/webhooks/interfaces.py	2015-07-29 09:06:47 +0000
@@ -28,6 +28,7 @@
     export_as_webservice_entry,
     export_destructor_operation,
     export_factory_operation,
+    export_write_operation,
     exported,
     operation_for_version,
     REQUEST_USER,
@@ -226,6 +227,16 @@
         title=_('Event payload'),
         key_type=TextLine(), required=True, readonly=True))
 
+    @export_write_operation()
+    @operation_for_version("devel")
+    def retry():
+        """Attempt to deliver the event again.
+
+        Launchpad will automatically retry regularly for 24 hours, but
+        this can be used after it gives up or to avoid waiting for the
+        next automatic attempt.
+        """
+
 
 class IWebhookDeliveryJobSource(IJobSource):
 

=== modified file 'lib/lp/services/webhooks/model.py'
--- lib/lp/services/webhooks/model.py	2015-07-29 09:06:46 +0000
+++ lib/lp/services/webhooks/model.py	2015-07-29 09:06:47 +0000
@@ -294,6 +294,7 @@
     class_job_type = WebhookJobType.DELIVERY
 
     retry_error_types = (WebhookDeliveryRetry,)
+    user_error_types = (WebhookDeliveryFailure,)
 
     # Effectively infinite, as we give up by checking
     # retry_automatically and raising a fatal exception instead.
@@ -351,6 +352,14 @@
     def _time_since_first_attempt(self):
         return datetime.now(utc) - (self.date_first_sent or self.date_created)
 
+    def retry(self):
+        """See `IWebhookDeliveryJob`."""
+        # Unset any retry delay and reset attempt_count to prevent
+        # queue() from delaying it again.
+        self.scheduled_start = None
+        self.attempt_count = 0
+        self.queue()
+
     @property
     def retry_automatically(self):
         return self._time_since_first_attempt < timedelta(days=1)

=== modified file 'lib/lp/services/webhooks/tests/test_webhookjob.py'
--- lib/lp/services/webhooks/tests/test_webhookjob.py	2015-07-29 09:06:46 +0000
+++ lib/lp/services/webhooks/tests/test_webhookjob.py	2015-07-29 09:06:47 +0000
@@ -21,6 +21,7 @@
     GreaterThan,
     Is,
     KeysEqual,
+    LessThan,
     MatchesAll,
     MatchesStructure,
     Not,
@@ -337,28 +338,27 @@
             job.date_first_sent - timedelta(hours=24)).isoformat()
         self.assertFalse(job.retry_automatically)
 
+    def runJob(self, job):
+        with dbuser("webhookrunner"):
+            runner = JobRunner([job])
+            runner.runAll()
+        job.lease_expires = None
+        if len(runner.completed_jobs) == 1 and not runner.incomplete_jobs:
+            return True
+        if len(runner.incomplete_jobs) == 1 and not runner.completed_jobs:
+            return False
+        if not runner.incomplete_jobs and not runner.completed_jobs:
+            return None
+        raise Exception("Unexpected jobs.")
+
     def test_automatic_retries(self):
         hook = self.factory.makeWebhook()
         job = WebhookDeliveryJob.create(hook, payload={'foo': 'bar'})
-
         client = MockWebhookClient(response_status=404)
         self.useFixture(ZopeUtilityFixture(client, IWebhookClient))
 
-        def run_job(job):
-            with dbuser("webhookrunner"):
-                runner = JobRunner([job])
-                runner.runAll()
-            if len(runner.completed_jobs) == 1 and not runner.incomplete_jobs:
-                return True
-            if len(runner.incomplete_jobs) == 1 and not runner.completed_jobs:
-                job.lease_expires = None
-                return False
-            if not runner.incomplete_jobs and not runner.completed_jobs:
-                return None
-            raise Exception("Unexpected jobs.")
-
         # The first attempt fails but schedules a retry five minutes later.
-        self.assertEqual(False, run_job(job))
+        self.assertEqual(False, self.runJob(job))
         self.assertEqual(JobStatus.WAITING, job.status)
         self.assertEqual(False, job.successful)
         self.assertTrue(job.pending)
@@ -370,7 +370,7 @@
         job.json_data['date_first_sent'] = (
             job.date_first_sent - timedelta(minutes=5)).isoformat()
         job.scheduled_start -= timedelta(minutes=5)
-        self.assertEqual(False, run_job(job))
+        self.assertEqual(False, self.runJob(job))
         self.assertEqual(JobStatus.WAITING, job.status)
         self.assertEqual(False, job.successful)
         self.assertTrue(job.pending)
@@ -380,11 +380,56 @@
         job.json_data['date_first_sent'] = (
             job.date_first_sent - timedelta(hours=24)).isoformat()
         job.scheduled_start -= timedelta(hours=24)
-        self.assertEqual(False, run_job(job))
+        self.assertEqual(False, self.runJob(job))
         self.assertEqual(JobStatus.FAILED, job.status)
         self.assertEqual(False, job.successful)
         self.assertFalse(job.pending)
 
+    def test_manual_retries(self):
+        hook = self.factory.makeWebhook()
+        job = WebhookDeliveryJob.create(hook, payload={'foo': 'bar'})
+        client = MockWebhookClient(response_status=404)
+        self.useFixture(ZopeUtilityFixture(client, IWebhookClient))
+
+        # Simulate a first attempt failure.
+        self.assertEqual(False, self.runJob(job))
+        self.assertEqual(JobStatus.WAITING, job.status)
+        self.assertIsNot(None, job.scheduled_start)
+
+        # A manual retry brings the scheduled start forward.
+        job.retry()
+        self.assertEqual(JobStatus.WAITING, job.status)
+        self.assertIs(None, job.scheduled_start)
+
+        # Force the next attempt to fail hard by pretending it was more
+        # than 24 hours later.
+        job.json_data['date_first_sent'] = (
+            job.date_first_sent - timedelta(hours=24)).isoformat()
+        self.assertEqual(False, self.runJob(job))
+        self.assertEqual(JobStatus.FAILED, job.status)
+
+        # A manual retry brings the job out of FAILED and schedules it
+        # to run as soon as possible. If it fails again, it fails hard;
+        # the initial attempt more than 24 hours ago is remembered.
+        job.retry()
+        self.assertEqual(JobStatus.WAITING, job.status)
+        self.assertIs(None, job.scheduled_start)
+        self.assertEqual(False, self.runJob(job))
+        self.assertEqual(JobStatus.FAILED, job.status)
+
+        # A completed job can be retried just like a failed one. The
+        # endpoint may have erroneously returned a 200 without recording
+        # the event.
+        client.response_status = 200
+        job.retry()
+        self.assertEqual(JobStatus.WAITING, job.status)
+        self.assertEqual(True, self.runJob(job))
+        self.assertEqual(JobStatus.COMPLETED, job.status)
+        job.retry()
+        self.assertEqual(JobStatus.WAITING, job.status)
+        self.assertEqual(True, self.runJob(job))
+        self.assertEqual(JobStatus.COMPLETED, job.status)
+
 
 class TestViaCronscript(TestCaseWithFactory):
 

=== modified file 'lib/lp/services/webhooks/tests/test_webservice.py'
--- lib/lp/services/webhooks/tests/test_webservice.py	2015-07-29 09:06:46 +0000
+++ lib/lp/services/webhooks/tests/test_webservice.py	2015-07-29 09:06:47 +0000
@@ -170,6 +170,20 @@
                     'date_created': Not(Is(None)),
                     'date_sent': Is(None)})))
 
+    def test_retry(self):
+        with person_logged_in(self.owner):
+            self.delivery.start()
+            self.delivery.fail()
+        representation = self.webservice.get(
+            self.delivery_url, api_version='devel').jsonBody()
+        self.assertFalse(representation['pending'])
+        response = self.webservice.named_post(
+            self.delivery_url, 'retry', api_version='devel')
+        self.assertEqual(200, response.status)
+        representation = self.webservice.get(
+            self.delivery_url, api_version='devel').jsonBody()
+        self.assertTrue(representation['pending'])
+
 
 class TestWebhookTarget(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer