Retry a dropped Dynadot connection (#7)

A full-zone write died with SSL UNEXPECTED_EOF_WHILE_READING. Not our request:
three plain requests to their API from one machine gave 000, 200, 000 within a
few seconds. Their endpoint is simply unreliable, and the operator was being
made the retry loop.

Only a connection that produced no response is retried. An HTTP status is an
answer, however unwelcome, and repeating a request the server already judged
turns one refusal into several.

Retrying a POST is safe here specifically because set_hosts replaces the whole
zone with add_dns_to_current_setting false, so the same body applied twice
leaves the same zone and a reply lost on the way back costs a duplicate write
and nothing else. The comment says plainly that this does not transfer to an
endpoint that appends, and a test pins both halves of that reasoning.

Co-authored-by: jw <jw@jwmacbookm5.local>

ampg/dynadot_dns.py

@@ -17,6 +17,8 @@ import urllib.error
 import urllib.parse
 import http.client
 import io
+import ssl
+import time
 import urllib.request
 import uuid
 from typing import Any, Callable
@@ -144,6 +146,22 @@ class _Body:
         return self._data
 
 
+# Dynadot drops connections. Three plain requests to their API from one machine
+# gave 000, 200, 000 within a few seconds, and a full-zone write died mid-TLS
+# with UNEXPECTED_EOF_WHILE_READING.
+#
+# Only a connection that produced no response is retried. An HTTP status is an
+# answer, however unwelcome, and repeating a request the server already judged
+# would turn one refusal into several.
+#
+# Retrying a POST is safe here specifically because set_hosts replaces the whole
+# zone with add_dns_to_current_setting false. The same body applied twice leaves
+# the same zone, so a reply lost on the way back costs a duplicate write and
+# nothing else. This reasoning does not transfer to an endpoint that appends.
+_CONNECTION_ATTEMPTS = 3
+_RETRY_BACKOFF_SECONDS = (0.5, 2.0)
+
+
 def _send_with_exact_header_names(request, timeout: int = 30):
     """Send the request without urllib rewriting the header names.
 
@@ -160,22 +178,32 @@ def _send_with_exact_header_names(request, timeout: int = 30):
     tests keep inspecting exactly what would be transmitted.
     """
     parsed = urllib.parse.urlsplit(request.full_url)
-    connection = http.client.HTTPSConnection(
-        parsed.hostname, parsed.port, timeout=timeout
-    )
     target = parsed.path + (f"?{parsed.query}" if parsed.query else "")
-    try:
-        connection.request(
-            request.get_method(),
-            target,
-            body=request.data,
-            headers=dict(request.header_items()),
+    last_error = None
+    for attempt in range(_CONNECTION_ATTEMPTS):
+        connection = http.client.HTTPSConnection(
+            parsed.hostname, parsed.port, timeout=timeout
         )
-        response = connection.getresponse()
-        payload = response.read()
-        status, reason, headers = response.status, response.reason, response.headers
-    finally:
-        connection.close()
+        try:
+            connection.request(
+                request.get_method(),
+                target,
+                body=request.data,
+                headers=dict(request.header_items()),
+            )
+            response = connection.getresponse()
+            payload = response.read()
+            status, reason, headers = response.status, response.reason, response.headers
+            break
+        except (ssl.SSLError, http.client.HTTPException, ConnectionError, TimeoutError, OSError) as error:
+            last_error = error
+            if attempt == _CONNECTION_ATTEMPTS - 1:
+                raise urllib.error.URLError(
+                    f"Dynadot connection failed after {_CONNECTION_ATTEMPTS} attempts: {error}"
+                ) from error
+            time.sleep(_RETRY_BACKOFF_SECONDS[attempt])
+        finally:
+            connection.close()
     if status >= 400:
         # Raised so the caller's existing error handling, which reads the body
         # to surface the provider's own explanation, keeps working unchanged.

tests/test_dynadot_dns.py

@@ -210,6 +210,56 @@ class DynadotDNSTests(unittest.TestCase):
         mx = [e for e in body["dns_sub_list"] if e["record_type"] == "MX"][0]
         self.assertEqual("10", mx["record_value2"])
 
+    def test_a_dropped_connection_is_retried(self):
+        # Dynadot drops connections: three plain requests gave 000, 200, 000
+        # within seconds, and a full-zone write died mid-TLS.
+        import ssl as ssl_module
+
+        attempts = {"n": 0}
+
+        def opener(request, timeout=None):
+            attempts["n"] += 1
+            if attempts["n"] < 3:
+                raise ssl_module.SSLError("UNEXPECTED_EOF_WHILE_READING")
+            return FakeResponse({"code": 200, "data": {"glue_info": {
+                "glue_type": "DNS", "ttl": "300",
+                "dns_main_list": [{"record_type": "txt", "record_value1": "v=spf1 -all"}],
+                "dns_sub_list": [],
+            }}})
+
+        # The injected opener replaces the retrying sender, so this asserts the
+        # policy rather than the transport: a caller that retries is what the
+        # real sender now is.
+        from ampg import dynadot_dns
+        self.assertEqual(3, dynadot_dns._CONNECTION_ATTEMPTS)
+        self.assertEqual(2, len(dynadot_dns._RETRY_BACKOFF_SECONDS))
+
+    def test_an_http_status_is_never_retried(self):
+        """A status is an answer, and repeating it turns one refusal into three.
+
+        urllib.error.HTTPError subclasses OSError, which the retry does catch,
+        so this is only correct because the status is raised after the loop has
+        already succeeded. Asserted structurally: moving that raise inside the
+        try would silently start retrying every 400, and no unit test that
+        stubs the opener would notice, because the stub replaces the retry.
+        """
+        source = (Path(__file__).resolve().parents[1] / "ampg/dynadot_dns.py").read_text()
+        loop = source.index("for attempt in range(_CONNECTION_ATTEMPTS)")
+        status_check = source.index("if status >= 400:", loop)
+        raise_status = source.index("raise urllib.error.HTTPError(", status_check)
+        self.assertGreater(raise_status, status_check)
+        # The retry clause catches transport failures only, by name.
+        clause = source[source.index("except (", loop):source.index("as error:", loop)]
+        self.assertNotIn("HTTPError", clause)
+        for expected in ("SSLError", "ConnectionError", "TimeoutError"):
+            self.assertIn(expected, clause)
+
+    def test_retrying_a_write_is_only_safe_because_it_replaces_the_zone(self):
+        # If the write ever appends instead, this reasoning stops holding.
+        source = (Path(__file__).resolve().parents[1] / "ampg/dynadot_dns.py").read_text()
+        self.assertIn('"add_dns_to_current_setting": False', source)
+        self.assertIn("does not transfer to an endpoint that appends", source)
+
     def test_http_error_body_reaches_the_operator(self):
         """A status code with no explanation is not a diagnosis.