diff --git a/skills/last30days/scripts/lib/http.py b/skills/last30days/scripts/lib/http.py index 06a69a2..8afd464 100644 --- a/skills/last30days/scripts/lib/http.py +++ b/skills/last30days/scripts/lib/http.py @@ -132,7 +132,9 @@ def request( if rate_limit_count >= max_429_retries: raise last_error - if attempt < effective_retries - 1: + # HTTP errors respect the caller's original `retries`; only DNS + # failures get the widened `effective_retries` budget. + if attempt < retries - 1: if e.code == 429: # Respect Retry-After header, fall back to exponential backoff retry_after = e.headers.get("Retry-After") if hasattr(e, 'headers') else None @@ -143,10 +145,15 @@ def request( delay = RETRY_DELAY * (2 ** attempt) + 1 else: delay = RETRY_DELAY * (2 ** attempt) + 1 # 3s, 5s, 9s... - log(f"Rate limited (429). Waiting {delay:.1f}s before retry {attempt + 2}/{effective_retries}") + log(f"Rate limited (429). Waiting {delay:.1f}s before retry {attempt + 2}/{retries}") else: delay = RETRY_DELAY * (2 ** attempt) time.sleep(delay) + else: + # Caller's original retry budget exhausted; an earlier DNS + # failure may have widened `effective_retries`, but that + # widening is DNS-only — don't grant extra HTTP attempts. + break except urllib.error.URLError as e: log(f"URL Error: {e.reason}") last_error = HTTPError(f"URL Error: {e.reason}") @@ -170,8 +177,15 @@ def request( f"retrying in {delay:.1f}s" ) time.sleep(delay) - elif attempt < effective_retries - 1: + elif attempt < retries - 1: + # Non-DNS URLError (e.g. ConnectionRefused) respects the + # caller's original retry budget, not the DNS-widened bound. time.sleep(RETRY_DELAY * (attempt + 1)) + else: + # Caller's original retry budget exhausted; an earlier DNS + # failure widening `effective_retries` does not carry over + # to non-DNS error paths. + break except json.JSONDecodeError as e: log(f"JSON decode error: {e}") last_error = HTTPError(f"Invalid JSON response: {e}") @@ -180,8 +194,12 @@ def request( # Handle socket-level errors (connection reset, timeout, etc.) log(f"Connection error: {type(e).__name__}: {e}") last_error = HTTPError(f"Connection error: {type(e).__name__}: {e}") - if attempt < effective_retries - 1: + if attempt < retries - 1: + # Socket errors respect the caller's original retry budget. time.sleep(RETRY_DELAY * (attempt + 1)) + else: + # Original budget exhausted; DNS widening doesn't apply here. + break attempt += 1 diff --git a/tests/test_http_v3.py b/tests/test_http_v3.py index 240efcd..9fbb876 100644 --- a/tests/test_http_v3.py +++ b/tests/test_http_v3.py @@ -180,3 +180,41 @@ class TestDNSResolutionRetry(unittest.TestCase): # Caller passed retries=2, and non-DNS URLError doesn't expand it. self.assertEqual(mock_urlopen.call_count, 2) + + @patch("lib.http.urllib.request.urlopen") + @patch("lib.http.time.sleep") + def test_dns_widening_does_not_leak_into_subsequent_non_dns_urlerror( + self, mock_sleep, mock_urlopen + ): + """Mixed sequence: DNS-then-non-DNS must respect caller's original retries. + + Without the fix, the first gaierror widens effective_retries from 2 to + MIN_DNS_RETRIES=3, and a subsequent ConnectionRefused on attempt 1 + slips into a third overall attempt — exceeding what the caller asked + for. Each non-DNS error path must gate on the original `retries`. + """ + import socket + dns_err = urllib.error.URLError(socket.gaierror(-2, "Name or service not known")) + conn_err = urllib.error.URLError(ConnectionRefusedError(111, "Connection refused")) + mock_urlopen.side_effect = [dns_err, conn_err, conn_err] # 3rd would only fire if budget leaked + + with self.assertRaises(http.HTTPError): + http.request("GET", "http://flaky.example", retries=2) + + # Caller asked for at most 2 attempts. DNS widening must not give us a 3rd. + self.assertEqual(mock_urlopen.call_count, 2) + + @patch("lib.http.urllib.request.urlopen") + @patch("lib.http.time.sleep") + def test_dns_widening_does_not_leak_into_subsequent_oserror( + self, mock_sleep, mock_urlopen + ): + """Mixed sequence: DNS-then-OSError must respect caller's original retries.""" + import socket + dns_err = urllib.error.URLError(socket.gaierror(-2, "Name or service not known")) + mock_urlopen.side_effect = [dns_err, TimeoutError("timed out"), TimeoutError("timed out")] + + with self.assertRaises(http.HTTPError): + http.request("GET", "http://flaky.example", retries=2) + + self.assertEqual(mock_urlopen.call_count, 2)