sawz/last30days-skill
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #382 from lustrousgorilla/bugfix/reddit-gaierror-retry

Browse Source 
fix(http): expand retry budget + exponential backoff on DNS resolution failure
This commit is contained in:
Trevin Chow
2026-05-16 22:11:56 -07:00
committed by GitHub
parent 89c5cb9d5d 719cdef2fb
commit b1c5f8db82
2 changed files with 173 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
skills/last30days/scripts/lib/http.py
+59 -2
View File
@@ -2,6 +2,7 @@
import json
import re
import socket
import sys
import time
import urllib.error
@@ -22,9 +23,19 @@ def log(msg: str):
MAX_RETRIES = 5
MAX_429_RETRIES = 2
RETRY_DELAY = 2.0
# DNS resolution failures (gaierror) are transient — typically resolved by a
# brief backoff and retry. Use a dedicated minimum attempt count + exponential
# delays (1s, 2s, 4s) so callers that pass a small `retries` value still get a
# meaningful chance to recover from a transient resolution failure.
MIN_DNS_RETRIES = 3
USER_AGENT = "last30days-skill/3.0 (Assistant Skill)"
def _is_dns_failure(err: urllib.error.URLError) -> bool:
"""Return True if a URLError was caused by DNS resolution (gaierror)."""
return isinstance(getattr(err, "reason", None), socket.gaierror)
class HTTPError(Exception):
"""HTTP request error with status code."""
def __init__(self, message: str, status_code: Optional[int] = None, body: Optional[str] = None):
@@ -85,7 +96,13 @@ def request(
last_error = None
rate_limit_count = 0
for attempt in range(retries):
# DNS failures get a dedicated minimum attempt count + exponential backoff.
# `effective_retries` is the actual loop bound; we expand it on the first
# gaierror if the caller passed a smaller `retries` value than MIN_DNS_RETRIES.
effective_retries = retries
dns_attempts = 0
attempt = 0
while attempt < effective_retries:
try:
with urllib.request.urlopen(req, timeout=timeout) as response:
body = response.read().decode('utf-8')
@@ -115,6 +132,8 @@ def request(
if rate_limit_count >= max_429_retries:
raise last_error
# 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
@@ -130,11 +149,43 @@ def request(
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}")
if attempt < retries - 1:
if _is_dns_failure(e):
# DNS resolution failures are transient; expand the retry budget
# to MIN_DNS_RETRIES if the caller passed fewer, and use
# exponential backoff (1s, 2s, 4s, ...) instead of the linear
# default. Counts DNS attempts separately so other URLError
# causes don't bypass the regular retry budget.
dns_attempts += 1
if effective_retries < MIN_DNS_RETRIES:
log(
f"DNS resolution failed; expanding retry budget from "
f"{effective_retries} to {MIN_DNS_RETRIES}"
)
effective_retries = MIN_DNS_RETRIES
if attempt < effective_retries - 1:
delay = 2 ** (dns_attempts - 1) # 1s, 2s, 4s, 8s, ...
log(
f"DNS resolution failure (attempt {dns_attempts}); "
f"retrying in {delay:.1f}s"
)
time.sleep(delay)
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}")
@@ -144,7 +195,13 @@ def request(
log(f"Connection error: {type(e).__name__}: {e}")
last_error = HTTPError(f"Connection error: {type(e).__name__}: {e}")
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
if last_error:
raise last_error
tests/test_http_v3.py
+114
View File
@@ -104,3 +104,117 @@ class TestParamsEncoding(unittest.TestCase):
sent_url = self._sent_url(mock_urlopen)
self.assertIn("count=25", sent_url)
self.assertIn("raw=True", sent_url)
class TestDNSResolutionRetry(unittest.TestCase):
"""DNS resolution failures (gaierror) must retry with exponential backoff.
Caller-passed `retries` values smaller than MIN_DNS_RETRIES are expanded
on the first gaierror so a transient resolution failure doesn't wipe a
request just because the caller passed retries=2.
"""
@patch("lib.http.urllib.request.urlopen")
@patch("lib.http.time.sleep")
def test_gaierror_retries_up_to_min_dns_retries_even_when_caller_passes_fewer(
self, mock_sleep, mock_urlopen
):
"""Caller passed retries=2; gaierror should still get MIN_DNS_RETRIES attempts."""
import socket
err = urllib.error.URLError(socket.gaierror(-2, "Name or service not known"))
mock_urlopen.side_effect = err
with self.assertRaises(http.HTTPError):
http.request("GET", "http://nonexistent.example", retries=2)
# Caller passed retries=2, but the budget expanded to MIN_DNS_RETRIES=3.
self.assertEqual(mock_urlopen.call_count, http.MIN_DNS_RETRIES)
@patch("lib.http.urllib.request.urlopen")
@patch("lib.http.time.sleep")
def test_gaierror_succeeds_after_transient_failure(self, mock_sleep, mock_urlopen):
"""gaierror on attempt 1, then success — should NOT raise."""
import socket
success_response = MagicMock()
success_response.read.return_value = b'{"ok": true}'
success_response.status = 200
success_response.__enter__ = lambda self: self
success_response.__exit__ = lambda *args: None
err = urllib.error.URLError(socket.gaierror(-2, "Name or service not known"))
mock_urlopen.side_effect = [err, success_response]
result = http.request("GET", "http://flaky.example", retries=2)
self.assertEqual(result, {"ok": True})
self.assertEqual(mock_urlopen.call_count, 2)
@patch("lib.http.urllib.request.urlopen")
@patch("lib.http.time.sleep")
def test_gaierror_uses_exponential_backoff(self, mock_sleep, mock_urlopen):
"""Backoff delays for gaierror should be 1s, 2s, 4s — not the linear default."""
import socket
err = urllib.error.URLError(socket.gaierror(-2, "Name or service not known"))
mock_urlopen.side_effect = err
with self.assertRaises(http.HTTPError):
http.request("GET", "http://nonexistent.example", retries=3)
# Expected sleep calls: 1s (after attempt 1), 2s (after attempt 2).
# No sleep after the final attempt (the loop exits to raise).
sleep_delays = [call.args[0] for call in mock_sleep.call_args_list]
self.assertEqual(sleep_delays, [1, 2])
@patch("lib.http.urllib.request.urlopen")
@patch("lib.http.time.sleep")
def test_non_dns_urlerror_uses_linear_backoff_not_dns_branch(
self, mock_sleep, mock_urlopen
):
"""A URLError that's NOT a gaierror must NOT expand the retry budget."""
# ConnectionRefusedError-style URLError reason (not gaierror)
err = urllib.error.URLError(ConnectionRefusedError(111, "Connection refused"))
mock_urlopen.side_effect = err
with self.assertRaises(http.HTTPError):
http.request("GET", "http://refused.example", retries=2)
# 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)