Disable retries when testing webhook integration

Normally, when a webhook call fails (timeout, connection
error, non-2xx response), the HTTP request is retried up to two
times (so up to 3 times total). This is useful when sending
actual notifications, in case the webhook target has a temporary
glitch.

When interactively testing a webhook integration
("Send Test Notification" in the
"Integrations" page), we would prefer to see any errors ASAP
on the screen instead of retrying and so possibly swallowing them.

One specific use case is webhook targets that take long time to
generate a response. "Send Test Notification" is synchronous,
meaning that the user could be stuck for
5 x 3 = 15 seconds waiting for the  test HTTP request to time out
three times.

CHANGELOG.md

@@ -11,6 +11,7 @@ All notable changes to this project will be documented in this file.
 - Reduce the number of SQL queries used in the "Get Checks" API call
 - Add support for script's exit status in ping URLs (#429)
 - Improve phone number sanitization: remove spaces and hyphens
+- Change the "Test Integration" behavior for webhooks: don't retry failed requests
 
 ## v1.17.0 - 2020-10-14
 

hc/api/models.py

@@ -489,7 +489,8 @@ class Channel(models.Model):
         n.save()
 
         # These are not database fields. It is just a convenient way to pass
-        # status_url to transport classes.
+        # status_url and the is_test flag to transport classes.
+        check.is_test = is_test
         check.status_url = n.status_url()
 
         error = self.transport.notify(check) or ""

hc/api/tests/test_notify.py

@@ -58,6 +58,9 @@ class NotifyTestCase(BaseTestCase):
         self._setup_data("webhook", json.dumps(definition))
         self.channel.notify(self.check)
 
+        # The transport should have retried 3 times
+        self.assertEqual(mock_get.call_count, 3)
+
         n = Notification.objects.get()
         self.assertEqual(n.error, "Connection timed out")
 
@@ -75,6 +78,9 @@ class NotifyTestCase(BaseTestCase):
         self._setup_data("webhook", json.dumps(definition))
         self.channel.notify(self.check)
 
+        # The transport should have retried 3 times
+        self.assertEqual(mock_get.call_count, 3)
+
         n = Notification.objects.get()
         self.assertEqual(n.error, "Connection failed")
 
@@ -92,9 +98,30 @@ class NotifyTestCase(BaseTestCase):
 
         self.channel.notify(self.check)
 
+        # The transport should have retried 3 times
+        self.assertEqual(mock_get.call_count, 3)
+
         n = Notification.objects.get()
         self.assertEqual(n.error, "Received status code 500")
 
+    @patch("hc.api.transports.requests.request", side_effect=Timeout)
+    def test_webhooks_dont_retry_when_sending_test_notifications(self, mock_get):
+        definition = {
+            "method_down": "GET",
+            "url_down": "http://example",
+            "body_down": "",
+            "headers_down": {},
+        }
+
+        self._setup_data("webhook", json.dumps(definition))
+        self.channel.notify(self.check, is_test=True)
+
+        # is_test flag is set, the transport should not retry:
+        self.assertEqual(mock_get.call_count, 1)
+
+        n = Notification.objects.get()
+        self.assertEqual(n.error, "Connection timed out")
+
     @patch("hc.api.transports.requests.request")
     def test_webhooks_support_variables(self, mock_get):
         definition = {

hc/api/transports.py

@@ -169,9 +169,8 @@ class HttpTransport(Transport):
             return "Connection failed"
 
     @classmethod
-    def get(cls, url, **kwargs):
-        # Make 3 attempts--
-        for x in range(0, 3):
+    def get(cls, url, num_tries=3, **kwargs):
+        for x in range(0, num_tries):
             error = cls._request("get", url, **kwargs)
             if error is None:
                 break
@@ -179,9 +178,8 @@ class HttpTransport(Transport):
         return error
 
     @classmethod
-    def post(cls, url, **kwargs):
-        # Make 3 attempts--
-        for x in range(0, 3):
+    def post(cls, url, num_tries=3, **kwargs):
+        for x in range(0, num_tries):
             error = cls._request("post", url, **kwargs)
             if error is None:
                 break
@@ -189,9 +187,8 @@ class HttpTransport(Transport):
         return error
 
     @classmethod
-    def put(cls, url, **kwargs):
-        # Make 3 attempts--
-        for x in range(0, 3):
+    def put(cls, url, num_tries=3, **kwargs):
+        for x in range(0, num_tries):
             error = cls._request("put", url, **kwargs)
             if error is None:
                 break
@@ -240,14 +237,19 @@ class Webhook(HttpTransport):
 
         body = spec["body"]
         if body:
-            body = self.prepare(body, check)
+            body = self.prepare(body, check).encode()
+
+        num_tries = 3
+        if getattr(check, "is_test"):
+            # When sending a test notification, don't retry on failures.
+            num_tries = 1
 
         if spec["method"] == "GET":
-            return self.get(url, headers=headers)
+            return self.get(url, num_tries=num_tries, headers=headers)
         elif spec["method"] == "POST":
-            return self.post(url, data=body.encode(), headers=headers)
+            return self.post(url, num_tries=num_tries, data=body, headers=headers)
         elif spec["method"] == "PUT":
-            return self.put(url, data=body.encode(), headers=headers)
+            return self.put(url, num_tries=num_tries, data=body, headers=headers)
 
 
 class Slack(HttpTransport):